feat: design multi signature scheme system#20
Merged
tac0turtle merged 7 commits intomainfrom Mar 9, 2026
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds sender-type-based payload verification and wire-encoded TxContext support, dynamic payload verifier registry (including Ed25519 test verifier), a Transaction post-bootstrap hook, EOA mapping conflict-safety, gateway/mempool integration changes, testapp/dev tooling, and multiple tests for custom-sender flows. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Gateway as EthGateway
participant Registry as VerifierRegistry
participant Verifier as SignatureVerifierDyn
participant Mempool
Client->>Gateway: submit raw tx (wire-encoded or envelope)
alt Wire-encoded Custom
Gateway->>Gateway: detect wire magic, decode TxContext -> TxPayload::Custom
Gateway->>Registry: verify_payload(sender_type, TxPayload::Custom)
else EOA Envelope
Gateway->>Gateway: decode TxEnvelope -> TxPayload::Eoa
Gateway->>Registry: verify_payload(EOA_SECP256K1, TxPayload::Eoa)
end
Registry->>Verifier: verify(&payload)
alt Verified
Verifier-->>Registry: Ok(())
Registry-->>Gateway: Ok(verified)
Gateway->>Mempool: submit verified TxContext
Mempool-->>Client: accepted
else Invalid / Unsupported
Verifier-->>Registry: Err
Registry-->>Gateway: Err(GatewayError)
Gateway-->>Client: reject
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
docker-compose.testapp.yml (1)
2-26: Consider adding a health check and restart policy.For improved reliability during development and CI testing, you may want to add:
healthcheck: test: ["CMD", "curl", "-f", "http://localhost:8545"] interval: 10s timeout: 5s retries: 3 restart: unless-stoppedThis helps with container orchestration and ensures the service recovers from transient failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.testapp.yml` around lines 2 - 26, Add a container healthcheck and restart policy to the testapp service in docker-compose.testapp.yml: under the service named "testapp" add a healthcheck that uses a CMD curl -f against http://localhost:8545 with interval 10s, timeout 5s and retries 3, and set restart to "unless-stopped" so the container is automatically restarted on transient failures; ensure these keys are placed alongside existing "ports", "environment", and "volumes" entries for the testapp service.bin/testapp/tests/mempool_e2e.rs (4)
577-581: Hardcoded nonce limits test reusability.The nonce is hardcoded to
0inEd25519AuthPayload. While this works for the single-transaction test, consider parameterizing it inEd25519CustomTxBuildInputfor future multi-transaction test scenarios.Suggested enhancement
struct Ed25519CustomTxBuildInput<'a> { tx_template_signer: &'a SigningKey, auth_signer: &'a Ed25519SigningKey, chain_id: u64, token_address: Address, recipient_account_id: AccountId, transfer_amount: u128, sender_account_id: AccountId, + nonce: u64, }Then use
input.nonceinstead of the literal0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/testapp/tests/mempool_e2e.rs` around lines 577 - 581, Ed25519AuthPayload is being constructed with a hardcoded nonce (0); change the code to take the nonce from the transaction builder input instead. Update the builder/input struct (Ed25519CustomTxBuildInput) to include a nonce field if it doesn't exist, pass that input through to the test, and replace the literal 0 with input.nonce when constructing auth_payload in the test where Ed25519AuthPayload and auth_payload are created so multi-transaction tests can supply different nonces.
802-806: Clarify the scope of the local verifier registry.The
SignatureVerifierRegistrycreated here is used only to verify the payload before submission, but it's not integrated into the actual block execution path (the STF). This is fine for testing the verifier logic in isolation, but consider adding a comment clarifying that the production path would need the registry configured in the STF/gateway.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/testapp/tests/mempool_e2e.rs` around lines 802 - 806, The local SignatureVerifierRegistry created here (SignatureVerifierRegistry::new(), registry.register_dyn(sender_types::CUSTOM, Ed25519PayloadVerifier), and the call to registry.verify_payload(sender_types::CUSTOM, tx_context.payload())) is only being used to test the verifier logic prior to submission and is not wired into the STF/block execution path; add a concise comment above this block stating that this registry is local to the test and that in production the verifier registry must be configured and injected into the STF/gateway so the block execution path uses the same verifiers.
91-105: Error code collision withEd25519PayloadVerifier.Error codes 0x71-0x74 defined here overlap with those used (or suggested) in
Ed25519PayloadVerifier(lines 59-77). If both components are used in the same error path, distinguishing the failure source becomes difficult.Consider using a separate range for account-level errors vs verifier-level errors, or documenting the error code allocation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/testapp/tests/mempool_e2e.rs` around lines 91 - 105, The error codes 0x71–0x74 in the functions err_invalid_auth_payload, err_nonce_mismatch, err_invalid_public_key, and err_invalid_signature collide with those used by Ed25519PayloadVerifier; change these account-level error functions to return codes from a distinct, non-overlapping range (e.g., bump to 0x80–0x83 or define an ACCOUNT_ERROR_BASE constant) and update any tests/comments accordingly so verifier vs account errors are unambiguous, or alternatively add a clear documented allocation comment that reserves 0x71–0x74 for verifier errors and reassign account errors to a different range.
56-79: Consider using distinct error codes for different failure modes.Lines 59, 62, 64, 65, 70, and 71 all return
ErrorCode::new(0x75)for different error conditions (invalid payload type, Borsh decode failures, missing invoke request, encode failure). This makes debugging difficult when errors occur.Suggested distinct error codes
impl SignatureVerifierDyn for Ed25519PayloadVerifier { fn verify(&self, payload: &TxPayload) -> SdkResult<()> { let TxPayload::Custom(bytes) = payload else { - return Err(ErrorCode::new(0x75)); + return Err(ErrorCode::new(0x70)); // Invalid payload type }; let intent: EthIntentPayload = - borsh::from_slice(bytes).map_err(|_| ErrorCode::new(0x75))?; + borsh::from_slice(bytes).map_err(|_| ErrorCode::new(0x71))?; // Intent decode error let decoded: Ed25519EthIntentProof = - borsh::from_slice(&intent.auth_proof).map_err(|_| ErrorCode::new(0x75))?; + borsh::from_slice(&intent.auth_proof).map_err(|_| ErrorCode::new(0x72))?; // Proof decode error let envelope = intent.decode_envelope().map_err(|_| ErrorCode::new(0x75))?; let invoke_request = envelope .to_invoke_requests() .into_iter() .next() - .ok_or_else(|| ErrorCode::new(0x75))?; - let request_digest = keccak256(&invoke_request.encode().map_err(|_| ErrorCode::new(0x75))?); + .ok_or_else(|| ErrorCode::new(0x73))?; // No invoke request + let request_digest = keccak256(&invoke_request.encode().map_err(|_| ErrorCode::new(0x74))?);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/testapp/tests/mempool_e2e.rs` around lines 56 - 79, The verify implementation in Ed25519PayloadVerifier (impl SignatureVerifierDyn for Ed25519PayloadVerifier -> fn verify) collapses multiple failure modes into ErrorCode::new(0x75), hindering debugging; update each failure site (the TxPayload match arm for TxPayload::Custom, both borsh::from_slice calls for EthIntentPayload and Ed25519EthIntentProof, intent.decode_envelope(), to_invoke_requests().next() missing-case, and invoke_request.encode()) to return distinct ErrorCode values (e.g., 0x75..0x7A) so callers can distinguish invalid payload type, intent decode error, auth_proof decode error, envelope decode error, missing invoke request, and encode error respectively; change the Err(...) and map_err(...) invocations to use the new unique ErrorCode::new(...) values while keeping existing shapes and mapping locations intact (match arm, borsh::from_slice map_err, decode_envelope map_err, ok_or_else for next, encode map_err).docker/evd/Dockerfile (1)
9-12: The fallbackrustup toolchain installwithout arguments may not work as intended.When
rustup show active-toolchainfails (toolchain not installed), the fallbackrustup toolchain installwithout specifying a toolchain name won't install anything meaningful. The command requires either a toolchain name argument or should rely on rustup's automatic toolchain installation.Consider using
rustup showwhich triggers auto-install fromrust-toolchain.toml:Proposed fix
-RUN rustup show active-toolchain || rustup toolchain install +RUN rustup showAlternatively, if you want explicit control:
RUN rustup toolchain install "$(cat rust-toolchain.toml | grep channel | cut -d'"' -f2)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/evd/Dockerfile` around lines 9 - 12, The fallback RUN line using "rustup show active-toolchain || rustup toolchain install" won't install a specific toolchain; update it to trigger auto-install from rust-toolchain.toml by replacing the fallback with a single "rustup show" (which consults rust-toolchain.toml) or explicitly install the channel parsed from rust-toolchain.toml; locate the COPY rust-toolchain.toml ./rust-toolchain.toml and the RUN rustup show active-toolchain || rustup toolchain install lines and either call rustup show alone or extract the "channel" from rust-toolchain.toml and pass it to rustup toolchain install so a concrete toolchain is installed.crates/app/tx/eth/src/eoa_registry.rs (1)
185-213: Potential inconsistent mapping state when forward mapping exists but reverse is missing.In
ensure_eoa_mapping, when the forward mapping (address -> account_id) exists and matches, but the reverse mapping (account_id -> address) does not exist (line 197 returnsNone), the function falls through to line 205 which checks only the reverse mapping. If the reverse doesn't exist, it creates both mappings viaset_mapping.However, at line 201, if the forward exists and matches, but the reverse also exists and matches, it returns
Ok(())early. This is correct.The issue is at line 202: if
lookup_address_in_envreturnsNone(reverse doesn't exist), the code exits the outerifblock and falls to line 205. This appears to be intentional fall-through behavior, but the nested structure makes it harder to follow.Consider simplifying the logic for clarity:
♻️ Suggested clearer structure
pub fn ensure_eoa_mapping( address: Address, account_id: AccountId, env: &mut dyn Environment, ) -> SdkResult<()> { - if let Some(existing) = lookup_account_id_in_env(address, env)? { - if existing != account_id { - return Err(ERR_ADDRESS_ACCOUNT_CONFLICT); - } - if let Some(existing_addr) = lookup_address_in_env(account_id, env)? { - if existing_addr != address { - return Err(ERR_ADDRESS_ACCOUNT_CONFLICT); - } - return Ok(()); - } - } - - if let Some(existing_addr) = lookup_address_in_env(account_id, env)? { - if existing_addr != address { - return Err(ERR_ADDRESS_ACCOUNT_CONFLICT); - } - return set_mapping(address, account_id, env); - } - - set_mapping(address, account_id, env) + let forward = lookup_account_id_in_env(address, env)?; + let reverse = lookup_address_in_env(account_id, env)?; + + // Validate consistency of any existing mappings + if let Some(existing_id) = forward { + if existing_id != account_id { + return Err(ERR_ADDRESS_ACCOUNT_CONFLICT); + } + } + if let Some(existing_addr) = reverse { + if existing_addr != address { + return Err(ERR_ADDRESS_ACCOUNT_CONFLICT); + } + } + + // If both mappings already exist and are consistent, nothing to do + if forward.is_some() && reverse.is_some() { + return Ok(()); + } + + // Create or complete the mapping + set_mapping(address, account_id, env) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/tx/eth/src/eoa_registry.rs` around lines 185 - 213, The current branching in ensure_eoa_mapping is confusing and can be simplified: first call lookup_account_id_in_env(address) and if Some(existing) and existing != account_id return ERR_ADDRESS_ACCOUNT_CONFLICT; then call lookup_address_in_env(account_id) and if Some(existing_addr) and existing_addr != address return ERR_ADDRESS_ACCOUNT_CONFLICT; if both lookups returned Some and they match return Ok(()); otherwise call set_mapping(address, account_id, env). Update ensure_eoa_mapping to use lookup_account_id_in_env, lookup_address_in_env, ERR_ADDRESS_ACCOUNT_CONFLICT, and set_mapping in that order to make the logic linear and avoid the nested fall-through.crates/app/tx/eth/src/gateway.rs (2)
163-184: Error mapping may obscure the actual failure reason.When
verify_payloadfails, the error is mapped by inspecting the chain ID post-hoc (lines 170-179). If the signature is invalid and the chain ID is wrong, this returnsInvalidChainIdrather thanInvalidSignature. This could mislead debugging efforts.Consider verifying chain ID before signature verification for clearer error semantics:
♻️ Suggested refactor
pub fn verify_envelope(&self, envelope: TxEnvelope) -> Result<TxContext, GatewayError> { + // Validate chain ID first for clearer error reporting + match envelope.chain_id() { + Some(id) if id != self.chain_id => { + return Err(GatewayError::InvalidChainId { + expected: self.chain_id, + actual: Some(id), + }); + } + None => { + return Err(GatewayError::InvalidChainId { + expected: self.chain_id, + actual: None, + }); + } + _ => {} + } + let payload = TxPayload::Eoa(Box::new(envelope.clone())); - // Verify chain ID and signature self.verifier .verify_payload(sender_type::EOA_SECP256K1, &payload) - .map_err(|_| match envelope.chain_id() { - Some(id) if id != self.chain_id => GatewayError::InvalidChainId { - expected: self.chain_id, - actual: Some(id), - }, - None => GatewayError::InvalidChainId { - expected: self.chain_id, - actual: None, - }, - _ => GatewayError::InvalidSignature, - })?; + .map_err(|_| GatewayError::InvalidSignature)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/tx/eth/src/gateway.rs` around lines 163 - 184, In verify_envelope, check the envelope chain ID against self.chain_id before calling self.verifier.verify_payload so chain-id mismatches return GatewayError::InvalidChainId deterministically; only if the chain ID matches (or is absent if that’s acceptable) proceed to call verify_payload and map its failure to GatewayError::InvalidSignature (or propagate the existing mapping for None/Some mismatches handled earlier); keep the final TxContext::new(envelope, self.base_fee).ok_or(GatewayError::ContractCreationNotSupported) unchanged.
187-218: Consider adding tests for new public API methods.The new
register_payload_verifier,supports_sender_type, andverify_contextmethods lack direct test coverage. While the underlying registry is tested separately, gateway-level integration tests would help ensure the wiring is correct.Would you like me to generate test cases for the new gateway methods?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/tx/eth/src/gateway.rs` around lines 187 - 218, Add integration tests in the existing tests module for EthGateway that exercise the new public API: call register_payload_verifier on an EthGateway instance to register a mock verifier and assert it is accepted (and errors handled), call supports_sender_type with known supported/unsupported sender types and assert the boolean result, and call verify_context with valid and invalid contexts to assert Ok and Err(GatewayError::VerifyFailed(_)) outcomes; use EthGateway::new or ::with_base_fee to construct, and reference the methods register_payload_verifier, supports_sender_type, and verify_context so the tests validate the gateway-level wiring rather than only the registry unit tests.crates/app/tx/eth/src/mempool.rs (3)
498-501: Envelope decode usesbase_fee=0, affectingeffective_gas_price.When decoding a raw envelope (non-wire format),
base_fee=0is used (line 500). For EIP-1559 transactions, this affects the calculatedeffective_gas_price. If this is intentional (e.g., for replay/recovery where ordering doesn't matter), consider adding a brief comment.+ // Envelope-only decode path uses zero base fee; effective_gas_price + // is recalculated for mempool insertion in decode_and_verify. let envelope = TxEnvelope::decode(bytes)?; TxContext::new(envelope, 0).ok_or(ERR_RECIPIENT_REQUIRED)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/tx/eth/src/mempool.rs` around lines 498 - 501, The TxEnvelope::decode + TxContext::new call currently constructs the TxContext with base_fee=0 which changes effective_gas_price calculation for EIP-1559 transactions; update the code near TxEnvelope::decode / TxContext::new to either compute/pass the correct base_fee derived from the envelope or (if zero is intentional for replay/recovery) add a concise comment explaining why base_fee=0 is used and the implication for effective_gas_price and ordering; reference TxEnvelope::decode, TxContext::new, effective_gas_price and ERR_RECIPIENT_REQUIRED when making the change so reviewers can quickly find and verify the intent.
330-336:recipient()returnsAccountId::invalid()forNoneresolution.When
recipient_resolutionisNone,recipient()returnsAccountId::invalid(). This sentinel value could propagate silently and cause subtle bugs if callers don't check for it.Consider whether this should panic (like
sender_address()) or whetherAccountId::invalid()is a well-defined sentinel that callers are expected to handle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/tx/eth/src/mempool.rs` around lines 330 - 336, The recipient() method currently returns AccountId::invalid() for RecipientResolution::None which can silently propagate; change it to mirror sender_address() behavior and fail loudly: replace the None branch to panic with a clear message (e.g. using panic! or expect on an Option) instead of returning AccountId::invalid(), so callers don't receive a silent sentinel; update the RecipientResolution::None handling in recipient() (and ensure any tests/call sites reflect the new panic semantics) while keeping the Account and EoaAddress branches using the existing account and derive_eth_eoa_account_id(address) logic.
236-243: Panic-based accessors may cause runtime failures.
sender_address()(line 240) andenvelope()(line 272) panic when called on incompatible payload types. While_optvariants exist, panics in library code can be surprising to callers.Consider either:
- Documenting these as
#[doc(hidden)]or internal-only- Returning
Resultinstead of panicking- Adding debug assertions in tests to catch misuse early
The current approach is acceptable if callers are expected to check
sender_type()orpayload()first, but the panic messages should be more actionable.Also applies to: 268-274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/tx/eth/src/mempool.rs` around lines 236 - 243, The accessor methods sender_address() and envelope() currently panic on incompatible SenderResolution/Payload types which can cause runtime failures; change them to return Result<T, Error> (e.g., Result<Address, MyError> / Result<EnvelopeType, MyError>) instead of panicking so callers must handle the mismatch, update call sites to propagate or handle the error, and add a clear, actionable error variant/message indicating the expected variant and the actual variant; alternatively, if these are truly internal-only, mark them #[doc(hidden)] and replace the panic text with a more descriptive message and/or add debug_assert! checks (but prefer the Result approach for public APIs).crates/app/tx/eth/src/verifier/registry.rs (1)
90-96: Backward-compatibleverifyallocates unnecessarily.The
verifymethod creates a newBoxallocation for every call viaBox::new(tx.clone()). For high-throughput scenarios, consider whether callers can migrate toverify_payloaddirectly.This is acceptable for a transitional compatibility shim, but the allocation overhead should be documented or the method marked as deprecated if migration is expected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/tx/eth/src/verifier/registry.rs` around lines 90 - 96, The compatibility shim register::verify currently allocates by cloning the TxEnvelope into a Box for every call (Box::new(tx.clone()) passed as TxPayload::Eoa) — mark this method as deprecated and document the allocation cost so callers migrate to verify_payload; specifically add a #[deprecated(...)] attribute to the verify function, update its doc comment to state it performs a heap allocation (via TxPayload::Eoa(Box::new(...))) and clearly recommend callers call verify_payload(sender_type::EOA_SECP256K1, &TxPayload::Eoa(...)) directly to avoid the clone/Box allocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rpc/evnode/src/service.rs`:
- Line 13: The import list in service.rs includes an unused symbol MempoolTx
from evolve_mempool; remove MempoolTx from the use statement (leave Mempool and
SharedMempool) so the compiler warning goes away and imports only used symbols;
update the line containing "use evolve_mempool::{Mempool, MempoolTx,
SharedMempool};" to exclude MempoolTx.
---
Nitpick comments:
In `@bin/testapp/tests/mempool_e2e.rs`:
- Around line 577-581: Ed25519AuthPayload is being constructed with a hardcoded
nonce (0); change the code to take the nonce from the transaction builder input
instead. Update the builder/input struct (Ed25519CustomTxBuildInput) to include
a nonce field if it doesn't exist, pass that input through to the test, and
replace the literal 0 with input.nonce when constructing auth_payload in the
test where Ed25519AuthPayload and auth_payload are created so multi-transaction
tests can supply different nonces.
- Around line 802-806: The local SignatureVerifierRegistry created here
(SignatureVerifierRegistry::new(), registry.register_dyn(sender_types::CUSTOM,
Ed25519PayloadVerifier), and the call to
registry.verify_payload(sender_types::CUSTOM, tx_context.payload())) is only
being used to test the verifier logic prior to submission and is not wired into
the STF/block execution path; add a concise comment above this block stating
that this registry is local to the test and that in production the verifier
registry must be configured and injected into the STF/gateway so the block
execution path uses the same verifiers.
- Around line 91-105: The error codes 0x71–0x74 in the functions
err_invalid_auth_payload, err_nonce_mismatch, err_invalid_public_key, and
err_invalid_signature collide with those used by Ed25519PayloadVerifier; change
these account-level error functions to return codes from a distinct,
non-overlapping range (e.g., bump to 0x80–0x83 or define an ACCOUNT_ERROR_BASE
constant) and update any tests/comments accordingly so verifier vs account
errors are unambiguous, or alternatively add a clear documented allocation
comment that reserves 0x71–0x74 for verifier errors and reassign account errors
to a different range.
- Around line 56-79: The verify implementation in Ed25519PayloadVerifier (impl
SignatureVerifierDyn for Ed25519PayloadVerifier -> fn verify) collapses multiple
failure modes into ErrorCode::new(0x75), hindering debugging; update each
failure site (the TxPayload match arm for TxPayload::Custom, both
borsh::from_slice calls for EthIntentPayload and Ed25519EthIntentProof,
intent.decode_envelope(), to_invoke_requests().next() missing-case, and
invoke_request.encode()) to return distinct ErrorCode values (e.g., 0x75..0x7A)
so callers can distinguish invalid payload type, intent decode error, auth_proof
decode error, envelope decode error, missing invoke request, and encode error
respectively; change the Err(...) and map_err(...) invocations to use the new
unique ErrorCode::new(...) values while keeping existing shapes and mapping
locations intact (match arm, borsh::from_slice map_err, decode_envelope map_err,
ok_or_else for next, encode map_err).
In `@crates/app/tx/eth/src/eoa_registry.rs`:
- Around line 185-213: The current branching in ensure_eoa_mapping is confusing
and can be simplified: first call lookup_account_id_in_env(address) and if
Some(existing) and existing != account_id return ERR_ADDRESS_ACCOUNT_CONFLICT;
then call lookup_address_in_env(account_id) and if Some(existing_addr) and
existing_addr != address return ERR_ADDRESS_ACCOUNT_CONFLICT; if both lookups
returned Some and they match return Ok(()); otherwise call set_mapping(address,
account_id, env). Update ensure_eoa_mapping to use lookup_account_id_in_env,
lookup_address_in_env, ERR_ADDRESS_ACCOUNT_CONFLICT, and set_mapping in that
order to make the logic linear and avoid the nested fall-through.
In `@crates/app/tx/eth/src/gateway.rs`:
- Around line 163-184: In verify_envelope, check the envelope chain ID against
self.chain_id before calling self.verifier.verify_payload so chain-id mismatches
return GatewayError::InvalidChainId deterministically; only if the chain ID
matches (or is absent if that’s acceptable) proceed to call verify_payload and
map its failure to GatewayError::InvalidSignature (or propagate the existing
mapping for None/Some mismatches handled earlier); keep the final
TxContext::new(envelope,
self.base_fee).ok_or(GatewayError::ContractCreationNotSupported) unchanged.
- Around line 187-218: Add integration tests in the existing tests module for
EthGateway that exercise the new public API: call register_payload_verifier on
an EthGateway instance to register a mock verifier and assert it is accepted
(and errors handled), call supports_sender_type with known supported/unsupported
sender types and assert the boolean result, and call verify_context with valid
and invalid contexts to assert Ok and Err(GatewayError::VerifyFailed(_))
outcomes; use EthGateway::new or ::with_base_fee to construct, and reference the
methods register_payload_verifier, supports_sender_type, and verify_context so
the tests validate the gateway-level wiring rather than only the registry unit
tests.
In `@crates/app/tx/eth/src/mempool.rs`:
- Around line 498-501: The TxEnvelope::decode + TxContext::new call currently
constructs the TxContext with base_fee=0 which changes effective_gas_price
calculation for EIP-1559 transactions; update the code near TxEnvelope::decode /
TxContext::new to either compute/pass the correct base_fee derived from the
envelope or (if zero is intentional for replay/recovery) add a concise comment
explaining why base_fee=0 is used and the implication for effective_gas_price
and ordering; reference TxEnvelope::decode, TxContext::new, effective_gas_price
and ERR_RECIPIENT_REQUIRED when making the change so reviewers can quickly find
and verify the intent.
- Around line 330-336: The recipient() method currently returns
AccountId::invalid() for RecipientResolution::None which can silently propagate;
change it to mirror sender_address() behavior and fail loudly: replace the None
branch to panic with a clear message (e.g. using panic! or expect on an Option)
instead of returning AccountId::invalid(), so callers don't receive a silent
sentinel; update the RecipientResolution::None handling in recipient() (and
ensure any tests/call sites reflect the new panic semantics) while keeping the
Account and EoaAddress branches using the existing account and
derive_eth_eoa_account_id(address) logic.
- Around line 236-243: The accessor methods sender_address() and envelope()
currently panic on incompatible SenderResolution/Payload types which can cause
runtime failures; change them to return Result<T, Error> (e.g., Result<Address,
MyError> / Result<EnvelopeType, MyError>) instead of panicking so callers must
handle the mismatch, update call sites to propagate or handle the error, and add
a clear, actionable error variant/message indicating the expected variant and
the actual variant; alternatively, if these are truly internal-only, mark them
#[doc(hidden)] and replace the panic text with a more descriptive message and/or
add debug_assert! checks (but prefer the Result approach for public APIs).
In `@crates/app/tx/eth/src/verifier/registry.rs`:
- Around line 90-96: The compatibility shim register::verify currently allocates
by cloning the TxEnvelope into a Box for every call (Box::new(tx.clone()) passed
as TxPayload::Eoa) — mark this method as deprecated and document the allocation
cost so callers migrate to verify_payload; specifically add a #[deprecated(...)]
attribute to the verify function, update its doc comment to state it performs a
heap allocation (via TxPayload::Eoa(Box::new(...))) and clearly recommend
callers call verify_payload(sender_type::EOA_SECP256K1, &TxPayload::Eoa(...))
directly to avoid the clone/Box allocation.
In `@docker-compose.testapp.yml`:
- Around line 2-26: Add a container healthcheck and restart policy to the
testapp service in docker-compose.testapp.yml: under the service named "testapp"
add a healthcheck that uses a CMD curl -f against http://localhost:8545 with
interval 10s, timeout 5s and retries 3, and set restart to "unless-stopped" so
the container is automatically restarted on transient failures; ensure these
keys are placed alongside existing "ports", "environment", and "volumes" entries
for the testapp service.
In `@docker/evd/Dockerfile`:
- Around line 9-12: The fallback RUN line using "rustup show active-toolchain ||
rustup toolchain install" won't install a specific toolchain; update it to
trigger auto-install from rust-toolchain.toml by replacing the fallback with a
single "rustup show" (which consults rust-toolchain.toml) or explicitly install
the channel parsed from rust-toolchain.toml; locate the COPY rust-toolchain.toml
./rust-toolchain.toml and the RUN rustup show active-toolchain || rustup
toolchain install lines and either call rustup show alone or extract the
"channel" from rust-toolchain.toml and pass it to rustup toolchain install so a
concrete toolchain is installed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
bin/testapp/Cargo.tomlbin/testapp/tests/mempool_e2e.rscrates/app/sdk/stf_traits/src/lib.rscrates/app/stf/src/lib.rscrates/app/tx/eth/src/eoa_registry.rscrates/app/tx/eth/src/error.rscrates/app/tx/eth/src/gateway.rscrates/app/tx/eth/src/lib.rscrates/app/tx/eth/src/mempool.rscrates/app/tx/eth/src/payload.rscrates/app/tx/eth/src/sender_type.rscrates/app/tx/eth/src/verifier/mod.rscrates/app/tx/eth/src/verifier/registry.rscrates/rpc/chain-index/src/provider.rscrates/rpc/evnode/src/service.rsdocker-compose.testapp.ymldocker/evd/Dockerfile
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/app/tx/eth/src/mempool.rs (2)
268-274: Same concern:envelope()panics for custom payloads.Similar to
sender_address(), this accessor panics when called onCustompayloads. Theenvelope_opt()method provides a safe alternative. Consider consolidating to avoid accidental panics in production code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/tx/eth/src/mempool.rs` around lines 268 - 274, The envelope() accessor currently panics for TxPayload::Custom just like sender_address() — replace or refactor it to avoid accidental panics by delegating to the safe envelope_opt() or changing envelope() to return Option<&TxEnvelope>; update the function body for envelope() (and any callers) to either return envelope_opt().expect_* only at call sites that truly guarantee Eoa payloads, or change envelope() to simply return envelope_opt() (Option) so callers must handle the None case, referencing the envelope(), envelope_opt(), and sender_address() symbols to locate and adjust the implementation and usages.
236-243: Consider returningResultinstead of panicking.The
sender_address()method panics when called on non-EOA payloads. While the method documentation mentions this limitation, a panic in production code could be problematic. Consider either:
- Making this method return
Option<Address>(similar tosender_address_opt)- Removing the panicking method and keeping only
sender_address_optSince
sender_address_optalready exists, the panicking variant may be unnecessary.♻️ Alternative: Remove panicking accessor
If callers always need to handle the
Nonecase anyway, consider deprecating or removingsender_address()in favor ofsender_address_opt()to prevent accidental panics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/tx/eth/src/mempool.rs` around lines 236 - 243, The panicking sender_address() should be made safe: change its signature to return Option<Address> and implement it by delegating to the existing sender_address_opt() (i.e., return self.sender_address_opt()), removing the panic path; update any callers to handle the Option (or, if you prefer removal instead, delete sender_address() and replace all usages with sender_address_opt()). Ensure references to SenderResolution::EoaAddress / SenderResolution::Account remain unchanged.crates/app/tx/eth/src/eoa_registry.rs (2)
185-213: Add in-file tests for the repair and conflict branches.This helper now owns the EOA mapping invariant, but the file still doesn't pin down the forward-only, reverse-only, and conflict cases. Please add unit tests next to this implementation.
As per coding guidelines, "Place unit tests alongside implementation in the same Rust file".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/tx/eth/src/eoa_registry.rs` around lines 185 - 213, Add inline unit tests in the same Rust file that exercise ensure_eoa_mapping for the forward-only, reverse-only, successful-idempotent, repair (one side missing), and conflict branches: create tests that set up a mock or test Environment, use lookup_account_id_in_env and lookup_address_in_env to assert initial state, call ensure_eoa_mapping with mismatched and matching pairs to trigger ERR_ADDRESS_ACCOUNT_CONFLICT and successful mapping, and verify set_mapping was called (or that both forward and reverse lookups return the expected Address/AccountId) after repair; place tests immediately below the ensure_eoa_mapping implementation and use the same helper functions (lookup_account_id_in_env, lookup_address_in_env, set_mapping) so they run as unit tests alongside the implementation.
188-213: Keep the registry invariant checks in one place.
ensure_eoa_mapping()repeats the same forward/reverse conflict checks thatset_mapping()already does. That duplicates the invariant and adds extra reads before every repair. I’d keep the checks in one place and make the lower-level helper write-only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/tx/eth/src/eoa_registry.rs` around lines 188 - 213, ensure_eoa_mapping duplicates forward/reverse conflict checks that set_mapping already enforces, causing unnecessary reads; remove the lookup_account_id_in_env/lookup_address_in_env calls and the duplicate conflict logic from ensure_eoa_mapping and make it a write-only wrapper that simply calls set_mapping(address, account_id, env) and returns its result, relying on set_mapping to perform and return ERR_ADDRESS_ACCOUNT_CONFLICT when appropriate; if set_mapping currently doesn't perform those checks, move the existing conflict checks into set_mapping (using lookup_account_id_in_env and lookup_address_in_env) and then simplify ensure_eoa_mapping to just call set_mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/testapp/tests/mempool_e2e.rs`:
- Around line 231-255: The formatting in init_ed25519_auth_storage (and the
preceding init_eth_auth_storage) is inconsistent with rustfmt; run `cargo fmt`
(or `just quality`) to reformat these functions so spacing/indentation and line
breaks match project style, ensuring expressions like let mut nonce_key =
account_id.as_bytes().to_vec(); nonce_key.push(0u8); data.insert(...); and the
pubkey_key insert lines are reformatted; verify the function signatures and
comment alignment for init_ed25519_auth_storage and the related data.insert
calls are corrected.
---
Nitpick comments:
In `@crates/app/tx/eth/src/eoa_registry.rs`:
- Around line 185-213: Add inline unit tests in the same Rust file that exercise
ensure_eoa_mapping for the forward-only, reverse-only, successful-idempotent,
repair (one side missing), and conflict branches: create tests that set up a
mock or test Environment, use lookup_account_id_in_env and lookup_address_in_env
to assert initial state, call ensure_eoa_mapping with mismatched and matching
pairs to trigger ERR_ADDRESS_ACCOUNT_CONFLICT and successful mapping, and verify
set_mapping was called (or that both forward and reverse lookups return the
expected Address/AccountId) after repair; place tests immediately below the
ensure_eoa_mapping implementation and use the same helper functions
(lookup_account_id_in_env, lookup_address_in_env, set_mapping) so they run as
unit tests alongside the implementation.
- Around line 188-213: ensure_eoa_mapping duplicates forward/reverse conflict
checks that set_mapping already enforces, causing unnecessary reads; remove the
lookup_account_id_in_env/lookup_address_in_env calls and the duplicate conflict
logic from ensure_eoa_mapping and make it a write-only wrapper that simply calls
set_mapping(address, account_id, env) and returns its result, relying on
set_mapping to perform and return ERR_ADDRESS_ACCOUNT_CONFLICT when appropriate;
if set_mapping currently doesn't perform those checks, move the existing
conflict checks into set_mapping (using lookup_account_id_in_env and
lookup_address_in_env) and then simplify ensure_eoa_mapping to just call
set_mapping.
In `@crates/app/tx/eth/src/mempool.rs`:
- Around line 268-274: The envelope() accessor currently panics for
TxPayload::Custom just like sender_address() — replace or refactor it to avoid
accidental panics by delegating to the safe envelope_opt() or changing
envelope() to return Option<&TxEnvelope>; update the function body for
envelope() (and any callers) to either return envelope_opt().expect_* only at
call sites that truly guarantee Eoa payloads, or change envelope() to simply
return envelope_opt() (Option) so callers must handle the None case, referencing
the envelope(), envelope_opt(), and sender_address() symbols to locate and
adjust the implementation and usages.
- Around line 236-243: The panicking sender_address() should be made safe:
change its signature to return Option<Address> and implement it by delegating to
the existing sender_address_opt() (i.e., return self.sender_address_opt()),
removing the panic path; update any callers to handle the Option (or, if you
prefer removal instead, delete sender_address() and replace all usages with
sender_address_opt()). Ensure references to SenderResolution::EoaAddress /
SenderResolution::Account remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a914c1b-6d6a-4e6a-8578-230e1e4e6df0
📒 Files selected for processing (7)
bin/testapp/tests/mempool_e2e.rscrates/app/tx/eth/src/eoa_registry.rscrates/app/tx/eth/src/gateway.rscrates/app/tx/eth/src/mempool.rscrates/rpc/evnode/src/service.rsdocker-compose.testapp.ymldocker/evd/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-compose.testapp.yml
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/testapp/tests/mempool_e2e.rs (1)
607-654:⚠️ Potential issue | 🟠 MajorThis “E2E” case still bypasses the ingress wiring.
The custom-sender flow manually calls
registry.verify_payload(...)and then pushes a prebuiltTxContextinto the mempool. That means this test stays green even if the real gateway/provider registration forsender_types::CUSTOMbreaks, which is one of the key integrations in this PR. Either route this through the production decode/verify entrypoint or rename/scope it as a post-verification execution test.Also applies to: 798-817
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/testapp/tests/mempool_e2e.rs` around lines 607 - 654, The test helper submit_context_and_produce_block currently bypasses the ingress wiring by constructing and inserting a prebuilt TxContext (used by submit_and_produce_block which calls EthGateway::new(...).decode_and_verify(...)), so update the test to exercise the real registration/verification path: replace the manual creation or direct mempool.insert of TxContext with the production ingress decode/verify entrypoint (i.e., call the registry/provider verify_payload flow or the same gateway decode_and_verify used by real ingress) so sender_types::CUSTOM registration is exercised; alternatively, if you intend this to be a post-verification unit scenario, rename the helpers (submit_context_and_produce_block -> submit_verified_context_and_produce_block) and add comments clarifying it does not test registration/ingress, ensuring tests at lines ~798-817 get the same treatment.
🧹 Nitpick comments (2)
bin/testapp/tests/mempool_e2e.rs (2)
57-79: Register these new error codes withdefine_error!.This file adds several ad hoc
ErrorCode::new(...)paths. The repo guideline requiresdefine_error!so the codes are registered at compile time and their ranges stay explicit.As per coding guidelines, "Define errors using the define_error! macro for compile-time error registration with error codes in ranges: 0x00-0x3F (validation), 0x40-0x7F (system), 0x80-0xBF (business logic)".
Also applies to: 92-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/testapp/tests/mempool_e2e.rs` around lines 57 - 79, The new Ed25519PayloadVerifier::verify method currently uses ad-hoc ErrorCode::new(...) calls (e.g., 0x75–0x7C) which must be declared with the define_error! macro; add a define_error! block that registers named error variants for each used code (matching the repo's ranges, e.g., system 0x40-0x7F) and then replace the inline ErrorCode::new(...) usages in verify (and the other occurrences around lines 92-106) with the corresponding named error variants (or map to them via ErrorCode::from or the defined identifiers) so all error codes are compile-time registered and follow the project's error-range convention.
752-845: Split this test below the 70-line limit.The setup, verifier bootstrap, submission, and state assertions are all in one block now. Pulling the initial-state reads and post-block assertions into helpers would keep the scenario easier to scan and align with the repo rule.
As per coding guidelines, "Keep functions to less than 70 lines to maintain bounded complexity".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/testapp/tests/mempool_e2e.rs` around lines 752 - 845, The test test_custom_sender_ed25519_transfer_e2e exceeds the 70-line guideline by including setup, verifier bootstrap, submission, and assertions in one block; refactor by extracting helper functions: create setup helper (e.g., setup_ed25519_sender_test) that wraps setup_genesis_with_ed25519_sender and returns (handles, genesis_accounts, sender_account_id, bob_account_id, auth_signer, alice_key, bob_key, token_address, sender_nonce_before, sender_balance_before, bob_balance_before), an assertion helper (e.g., assert_post_transfer_state) that reads nonce/balances and performs the three assert_eq! checks, and optionally a small helper for registry bootstrap/verification (e.g., bootstrap_and_verify_custom_sender) to register and verify payloads; replace the inline blocks in test_custom_sender_ed25519_transfer_e2e with calls to these helpers so the test body stays <70 lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/testapp/tests/mempool_e2e.rs`:
- Around line 57-79: The verifier currently checks intent.auth_proof against the
invoke-request digest in Ed25519PayloadVerifier::verify but
Ed25519AuthAccount::authenticate trusts payload.message_digest from the caller,
allowing mismatched signatures; fix by making both sides use the same canonical
digest: compute the canonical preimage (e.g., nonce + request_digest + sender
binding) when verifying the intent in Ed25519PayloadVerifier::verify and either
(A) pass that computed digest into Ed25519AuthAccount::authenticate (do not
accept payload.message_digest from caller), or (B) have
Ed25519AuthAccount::authenticate recompute the digest internally from the
intent/envelope (not from authentication_payload) and verify the account
signature against that recomputed digest; ensure the unique symbols involved are
Ed25519PayloadVerifier::verify, intent.auth_proof, request_digest,
Ed25519AuthAccount::authenticate, payload.message_digest and
authentication_payload so both checks validate the same preimage.
---
Outside diff comments:
In `@bin/testapp/tests/mempool_e2e.rs`:
- Around line 607-654: The test helper submit_context_and_produce_block
currently bypasses the ingress wiring by constructing and inserting a prebuilt
TxContext (used by submit_and_produce_block which calls
EthGateway::new(...).decode_and_verify(...)), so update the test to exercise the
real registration/verification path: replace the manual creation or direct
mempool.insert of TxContext with the production ingress decode/verify entrypoint
(i.e., call the registry/provider verify_payload flow or the same gateway
decode_and_verify used by real ingress) so sender_types::CUSTOM registration is
exercised; alternatively, if you intend this to be a post-verification unit
scenario, rename the helpers (submit_context_and_produce_block ->
submit_verified_context_and_produce_block) and add comments clarifying it does
not test registration/ingress, ensuring tests at lines ~798-817 get the same
treatment.
---
Nitpick comments:
In `@bin/testapp/tests/mempool_e2e.rs`:
- Around line 57-79: The new Ed25519PayloadVerifier::verify method currently
uses ad-hoc ErrorCode::new(...) calls (e.g., 0x75–0x7C) which must be declared
with the define_error! macro; add a define_error! block that registers named
error variants for each used code (matching the repo's ranges, e.g., system
0x40-0x7F) and then replace the inline ErrorCode::new(...) usages in verify (and
the other occurrences around lines 92-106) with the corresponding named error
variants (or map to them via ErrorCode::from or the defined identifiers) so all
error codes are compile-time registered and follow the project's error-range
convention.
- Around line 752-845: The test test_custom_sender_ed25519_transfer_e2e exceeds
the 70-line guideline by including setup, verifier bootstrap, submission, and
assertions in one block; refactor by extracting helper functions: create setup
helper (e.g., setup_ed25519_sender_test) that wraps
setup_genesis_with_ed25519_sender and returns (handles, genesis_accounts,
sender_account_id, bob_account_id, auth_signer, alice_key, bob_key,
token_address, sender_nonce_before, sender_balance_before, bob_balance_before),
an assertion helper (e.g., assert_post_transfer_state) that reads nonce/balances
and performs the three assert_eq! checks, and optionally a small helper for
registry bootstrap/verification (e.g., bootstrap_and_verify_custom_sender) to
register and verify payloads; replace the inline blocks in
test_custom_sender_ed25519_transfer_e2e with calls to these helpers so the test
body stays <70 lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1c14bf5-fb37-41e2-8788-b5b32bf45319
📒 Files selected for processing (1)
bin/testapp/tests/mempool_e2e.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/app/tx/eth/src/eoa_registry.rs (1)
220-236:⚠️ Potential issue | 🟠 MajorRepair the reverse mapping before returning an existing account.
If
lookup_account_id_in_env(address)succeeds whileaccount_id -> addressis missing, Line 224 returns immediately and never exercises the newensure_eoa_mappingpath. That leaves reverse lookups broken even though this PR adds repair semantics for exactly that case.Possible fix
pub fn resolve_or_create_eoa_account( address: Address, env: &mut dyn Environment, ) -> SdkResult<AccountId> { if let Some(account_id) = lookup_account_id_in_env(address, env)? { + ensure_eoa_mapping(address, account_id, env)?; return Ok(account_id); } let account_id = derive_eth_eoa_account_id(address);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/tx/eth/src/eoa_registry.rs` around lines 220 - 236, The function resolve_or_create_eoa_account returns early when lookup_account_id_in_env(address, env) yields Some(account_id), which skips the new repair step; modify the early-return branch to call ensure_eoa_mapping(address, account_id, env)? before returning so the reverse mapping is repaired when missing. Concretely, in resolve_or_create_eoa_account replace the current immediate return for Some(account_id) with a call to ensure_eoa_mapping(address, account_id, env)? followed by Ok(account_id), preserving error propagation.crates/rpc/chain-index/src/provider.rs (1)
220-236:⚠️ Potential issue | 🟠 MajorValidate that the injected gateway matches
config.chain_id.
with_mempool_and_gatewaynow accepts anyEthGateway, but ingress verification uses that gateway while the rest of the provider is configured withconfig.chain_id. A mismatched pair will accept transactions for one chain through an RPC provider serving another. This should fail fast here, or the constructor should take gateway configuration instead of a prebuilt gateway.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rpc/chain-index/src/provider.rs` around lines 220 - 236, Ensure the injected EthGateway matches the provider config by checking the gateway's chain id against config.chain_id inside with_mempool_and_gateway (before wrapping gateway in Arc). Call the gateway's chain id accessor (e.g., gateway.chain_id() or equivalent) and if it does not equal config.chain_id, fail fast — either return an error or panic with a clear message indicating the mismatch (include both ids). This prevents creating a provider that will verify/accept transactions for a different chain than config.chain_id.crates/rpc/chain-index/src/integration.rs (1)
109-149:⚠️ Potential issue | 🟠 MajorDon't let envelope-less payloads fall through as synthetic legacy transactions.
When
tx.envelope()isNone, this now returnsNone, and the callers fall back to the generic path inbuild_stored_transaction/build_stored_receipt. That path hard-codes legacy-looking fields liketx_type: 0,nonce: 0, and zeroed signature values, so the new custom-sender transactions will be indexed as if they were real legacy Ethereum transactions instead of non-envelope/custom payloads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rpc/chain-index/src/integration.rs` around lines 109 - 149, The helper ethereum_tx_fields may return None for envelope-less/custom-sender TxContext, but callers build_stored_transaction and build_stored_receipt currently treat that as a signal to populate hard-coded legacy defaults (tx_type: 0, nonce: 0, zeroed signature), which misindexes custom payloads; update build_stored_transaction and build_stored_receipt to detect when ethereum_tx_fields(tx) returns None and explicitly handle the “no envelope” case instead of falling back to legacy values—either propagate the None so stored transaction/receipt fields remain empty/null or set an explicit non-legacy marker (e.g., tx_type = None or a distinct sentinel) and avoid filling in signature/nonce/gas fields with legacy defaults; ensure any downstream consumers check for that None/sentinel and do not assume legacy formatting.
🧹 Nitpick comments (5)
bin/testapp/src/eth_eoa.rs (1)
147-155: Remove or retarget this unreachableTxContextfallback.
authenticate()already returns ontx.get::<TxContext>()at Lines 134-136, so this branch can never run. As written it does not add backward compatibility; it only duplicates sender checks and makes the auth flow harder to reason about. If you still need a legacy path here, deserialize the actual legacy payload type instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/testapp/src/eth_eoa.rs` around lines 147 - 155, The TxContext fallback block is unreachable because authenticate() already returns on tx.get::<TxContext>() earlier; remove the else-if branch that checks tx.get::<TxContext>() (the code that computes sender_bytes and compares to expected_address returning ERR_SENDER_MISMATCH) or replace it with a proper deserialization of the legacy payload type instead of calling tx.get::<TxContext>(); update any tests/usage accordingly so only the real legacy payload type is handled and ERR_SENDER_MISMATCH remains produced only from the canonical auth path.crates/app/tx/eth/src/mempool.rs (1)
243-246: Remove or document the redundantsender_address_optaccessor.
sender_address_opt()is identical tosender_address(). If this is for API compatibility, consider deprecating one. Otherwise, consolidate to reduce confusion.♻️ Proposed refactor
- /// Get sender address when available. - pub fn sender_address_opt(&self) -> Option<Address> { - self.sender_address() - } + /// Get sender address when available. + #[deprecated(note = "use sender_address() instead")] + pub fn sender_address_opt(&self) -> Option<Address> { + self.sender_address() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/tx/eth/src/mempool.rs` around lines 243 - 246, The accessor sender_address_opt() is redundant with sender_address(), so either remove sender_address_opt() or explicitly document/deprecate it for API compatibility: remove the duplicate function declaration (and update any callers to use sender_address()), or annotate sender_address_opt() with a #[deprecated] attribute and add doc comments pointing users to sender_address() while leaving the wrapper in place for backward compatibility; ensure function references in tests and other modules are updated accordingly.bin/testapp/tests/mempool_e2e.rs (2)
10-10: Clarify the purpose of#![allow(unexpected_cfgs)].This allow attribute suppresses warnings about unexpected
cfgattributes. If this is intentional (e.g., for conditional compilation used elsewhere), consider adding a comment explaining which cfg flags are expected. If not needed, remove it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/testapp/tests/mempool_e2e.rs` at line 10, The crate-level attribute `#![allow(unexpected_cfgs)]` suppresses warnings about unexpected cfg attributes; either remove this attribute if it’s unnecessary, or keep it but add a short comment immediately above `#![allow(unexpected_cfgs)]` that documents which conditional compilation flags (cfg names) are intentionally used elsewhere (e.g., feature flags or platform cfgs) so reviewers understand why the warning is silenced.
88-117: Coupling between intent proof and account authentication.The verifier validates against the public key embedded in
Ed25519EthIntentProof, whileEd25519AuthAccount::authenticatevalidates against the public key stored in account storage. For the system to be secure, these must be the same key. The test correctly uses the same signer for both (lines 621-628), but production implementations should document this invariant clearly or enforce it structurally.This relates to the past review concern about digest binding. The current implementation is correct for testing purposes since both sides compute
request_digestfrom the sameinvoke_request, but the architectural coupling between the two verification steps should be explicit in production code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/testapp/tests/mempool_e2e.rs` around lines 88 - 117, The verifier (SignatureVerifierDyn::verify in Ed25519PayloadVerifier) uses the public key from Ed25519EthIntentProof while account auth (Ed25519AuthAccount::authenticate) uses the account-stored key, creating an implicit invariant that they must match; make that invariant explicit by either enforcing the key binding or documenting and asserting it: update verify to fetch the account's expected verification key for the request sender (derived from the decoded envelope/invoke_request) and compare it to decoded.public_key (Ed25519EthIntentProof) failing if they differ, or add a clear assertion/log and a unit test that enforces the same-key requirement and references Ed25519PayloadVerifier::verify and Ed25519AuthAccount::authenticate so production code cannot bypass the coupling.crates/app/tx/eth/src/verifier/registry.rs (1)
117-124: Consider consolidating duplicate verification logic.The
SignatureVerifier<TxEnvelope>implementation duplicates the logic from the deprecatedverifymethod (lines 98-103). Consider having one call the other to reduce duplication.♻️ Proposed refactor
impl SignatureVerifier<TxEnvelope> for SignatureVerifierRegistry { fn verify_signature(&self, tx: &TxEnvelope) -> SdkResult<()> { - self.verify_payload( - sender_type::EOA_SECP256K1, - &TxPayload::Eoa(Box::new(tx.clone())), - ) + #[allow(deprecated)] + self.verify(tx) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/tx/eth/src/verifier/registry.rs` around lines 117 - 124, The verify_signature implementation on SignatureVerifier<TxEnvelope> duplicates the logic in the existing deprecated verify method; consolidate by having one delegate to the other to avoid duplication — update SignatureVerifierRegistry::verify_signature to call the existing verify(&self, tx: &TxEnvelope) (or vice versa) so the logic for creating the TxPayload::Eoa and calling self.verify_payload(sender_type::EOA_SECP256K1, ...) is only present in one place (reference the methods SignatureVerifier<TxEnvelope>::verify_signature, SignatureVerifierRegistry::verify, and the verify_payload helper).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/app/tx/eth/src/error.rs`:
- Around line 21-30: Revert the numeric code change so ERR_SENDER_MISMATCH keeps
its original code: set define_error!(ERR_SENDER_MISMATCH, 0x17, ...) and assign
ERR_UNSUPPORTED_SENDER_TYPE a new unused validation code (replace 0x17 with a
different free code) in define_error!(ERR_UNSUPPORTED_SENDER_TYPE, ...); update
only the numeric literals in those define_error! invocations so the external
error ABI remains stable.
---
Outside diff comments:
In `@crates/app/tx/eth/src/eoa_registry.rs`:
- Around line 220-236: The function resolve_or_create_eoa_account returns early
when lookup_account_id_in_env(address, env) yields Some(account_id), which skips
the new repair step; modify the early-return branch to call
ensure_eoa_mapping(address, account_id, env)? before returning so the reverse
mapping is repaired when missing. Concretely, in resolve_or_create_eoa_account
replace the current immediate return for Some(account_id) with a call to
ensure_eoa_mapping(address, account_id, env)? followed by Ok(account_id),
preserving error propagation.
In `@crates/rpc/chain-index/src/integration.rs`:
- Around line 109-149: The helper ethereum_tx_fields may return None for
envelope-less/custom-sender TxContext, but callers build_stored_transaction and
build_stored_receipt currently treat that as a signal to populate hard-coded
legacy defaults (tx_type: 0, nonce: 0, zeroed signature), which misindexes
custom payloads; update build_stored_transaction and build_stored_receipt to
detect when ethereum_tx_fields(tx) returns None and explicitly handle the “no
envelope” case instead of falling back to legacy values—either propagate the
None so stored transaction/receipt fields remain empty/null or set an explicit
non-legacy marker (e.g., tx_type = None or a distinct sentinel) and avoid
filling in signature/nonce/gas fields with legacy defaults; ensure any
downstream consumers check for that None/sentinel and do not assume legacy
formatting.
In `@crates/rpc/chain-index/src/provider.rs`:
- Around line 220-236: Ensure the injected EthGateway matches the provider
config by checking the gateway's chain id against config.chain_id inside
with_mempool_and_gateway (before wrapping gateway in Arc). Call the gateway's
chain id accessor (e.g., gateway.chain_id() or equivalent) and if it does not
equal config.chain_id, fail fast — either return an error or panic with a clear
message indicating the mismatch (include both ids). This prevents creating a
provider that will verify/accept transactions for a different chain than
config.chain_id.
---
Nitpick comments:
In `@bin/testapp/src/eth_eoa.rs`:
- Around line 147-155: The TxContext fallback block is unreachable because
authenticate() already returns on tx.get::<TxContext>() earlier; remove the
else-if branch that checks tx.get::<TxContext>() (the code that computes
sender_bytes and compares to expected_address returning ERR_SENDER_MISMATCH) or
replace it with a proper deserialization of the legacy payload type instead of
calling tx.get::<TxContext>(); update any tests/usage accordingly so only the
real legacy payload type is handled and ERR_SENDER_MISMATCH remains produced
only from the canonical auth path.
In `@bin/testapp/tests/mempool_e2e.rs`:
- Line 10: The crate-level attribute `#![allow(unexpected_cfgs)]` suppresses
warnings about unexpected cfg attributes; either remove this attribute if it’s
unnecessary, or keep it but add a short comment immediately above
`#![allow(unexpected_cfgs)]` that documents which conditional compilation flags
(cfg names) are intentionally used elsewhere (e.g., feature flags or platform
cfgs) so reviewers understand why the warning is silenced.
- Around line 88-117: The verifier (SignatureVerifierDyn::verify in
Ed25519PayloadVerifier) uses the public key from Ed25519EthIntentProof while
account auth (Ed25519AuthAccount::authenticate) uses the account-stored key,
creating an implicit invariant that they must match; make that invariant
explicit by either enforcing the key binding or documenting and asserting it:
update verify to fetch the account's expected verification key for the request
sender (derived from the decoded envelope/invoke_request) and compare it to
decoded.public_key (Ed25519EthIntentProof) failing if they differ, or add a
clear assertion/log and a unit test that enforces the same-key requirement and
references Ed25519PayloadVerifier::verify and Ed25519AuthAccount::authenticate
so production code cannot bypass the coupling.
In `@crates/app/tx/eth/src/mempool.rs`:
- Around line 243-246: The accessor sender_address_opt() is redundant with
sender_address(), so either remove sender_address_opt() or explicitly
document/deprecate it for API compatibility: remove the duplicate function
declaration (and update any callers to use sender_address()), or annotate
sender_address_opt() with a #[deprecated] attribute and add doc comments
pointing users to sender_address() while leaving the wrapper in place for
backward compatibility; ensure function references in tests and other modules
are updated accordingly.
In `@crates/app/tx/eth/src/verifier/registry.rs`:
- Around line 117-124: The verify_signature implementation on
SignatureVerifier<TxEnvelope> duplicates the logic in the existing deprecated
verify method; consolidate by having one delegate to the other to avoid
duplication — update SignatureVerifierRegistry::verify_signature to call the
existing verify(&self, tx: &TxEnvelope) (or vice versa) so the logic for
creating the TxPayload::Eoa and calling
self.verify_payload(sender_type::EOA_SECP256K1, ...) is only present in one
place (reference the methods SignatureVerifier<TxEnvelope>::verify_signature,
SignatureVerifierRegistry::verify, and the verify_payload helper).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55c27024-7bda-477b-aa04-aaae382c42b1
📒 Files selected for processing (12)
bin/testapp/Cargo.tomlbin/testapp/src/eth_eoa.rsbin/testapp/tests/mempool_e2e.rscrates/app/tx/eth/src/eoa_registry.rscrates/app/tx/eth/src/error.rscrates/app/tx/eth/src/lib.rscrates/app/tx/eth/src/mempool.rscrates/app/tx/eth/src/verifier/registry.rscrates/app/tx/eth/tests/integration_tests.rscrates/rpc/chain-index/src/integration.rscrates/rpc/chain-index/src/provider.rscrates/rpc/evnode/src/service.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/rpc/evnode/src/service.rs
Comment on lines
21
to
30
| define_error!( | ||
| ERR_SENDER_MISMATCH, | ||
| ERR_UNSUPPORTED_SENDER_TYPE, | ||
| 0x17, | ||
| "unsupported sender type {arg}" | ||
| ); | ||
| define_error!( | ||
| ERR_SENDER_MISMATCH, | ||
| 0x1A, | ||
| "recovered sender does not match account address" | ||
| ); |
There was a problem hiding this comment.
Keep ERR_SENDER_MISMATCH on its original numeric code.
Reassigning 0x17 from ERR_SENDER_MISMATCH to a different error changes the external error ABI. Anything matching numeric codes will now interpret old sender-mismatch failures as ERR_UNSUPPORTED_SENDER_TYPE. Prefer giving the new error a free validation code and leaving the existing one stable.
Possible fix
define_error!(
ERR_UNSUPPORTED_SENDER_TYPE,
- 0x17,
+ 0x1A,
"unsupported sender type {arg}"
);
define_error!(
ERR_SENDER_MISMATCH,
- 0x1A,
+ 0x17,
"recovered sender does not match account address"
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| define_error!( | |
| ERR_SENDER_MISMATCH, | |
| ERR_UNSUPPORTED_SENDER_TYPE, | |
| 0x17, | |
| "unsupported sender type {arg}" | |
| ); | |
| define_error!( | |
| ERR_SENDER_MISMATCH, | |
| 0x1A, | |
| "recovered sender does not match account address" | |
| ); | |
| define_error!( | |
| ERR_UNSUPPORTED_SENDER_TYPE, | |
| 0x1A, | |
| "unsupported sender type {arg}" | |
| ); | |
| define_error!( | |
| ERR_SENDER_MISMATCH, | |
| 0x17, | |
| "recovered sender does not match account address" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/app/tx/eth/src/error.rs` around lines 21 - 30, Revert the numeric code
change so ERR_SENDER_MISMATCH keeps its original code: set
define_error!(ERR_SENDER_MISMATCH, 0x17, ...) and assign
ERR_UNSUPPORTED_SENDER_TYPE a new unused validation code (replace 0x17 with a
different free code) in define_error!(ERR_UNSUPPORTED_SENDER_TYPE, ...); update
only the numeric literals in those define_error! invocations so the external
error ABI remains stable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Design and implement a multi signature scheme system that allows users to migrate verification schemes of their accounts and chains can add thier own custom schemes
Summary by CodeRabbit
New Features
Refactor
Bug Fixes / Improvements
Chores / Tests