Conversation
WalkthroughThe changes introduce a new method, Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant S as SignerType
participant R as Recovery Logic
C->>S: Call normalize(message, sig)
alt SignerType is not GcpKms
S-->>C: Return cloned signature
else SignerType is GcpKms
S->>S: Set sig.v = 0
S->>R: Attempt recovery (v=0)
alt Recovery successful
R-->>S: Address matches expected
S-->>C: Return normalized signature with v = 0
else Recovery failed
S->>S: Set sig.v = 1
S->>R: Attempt recovery (v=1)
alt Recovery successful
R-->>S: Address matches expected
S-->>C: Return normalized signature with v = 1
else Recovery fails
S-->>C: Return ProviderError
end
end
end
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| sig: &Signature, | ||
| ) -> Result<Signature, ethers::types::SignatureError> { | ||
| match self { | ||
| // Since GCP KMS doesn't produce signatures normalized for EVM, we need to correct v |
There was a problem hiding this comment.
/
| // Since GCP KMS doesn't produce signatures normalized for EVM, we need to correct v | |
| // Gravity does not implement eip155 modifications to v so we need to recompute v to the allowed range of 0 or 1 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
orchestrator/gravity/src/ethereum/types.rs (1)
18-55: Implementation looks good but could use minor optimizations.The
set_vmethod correctly handles the normalization of thevvalue for GCP KMS signatures. The approach of trying both possiblevvalues (0 and 1) is appropriate for EVM signature recovery.A few suggestions:
- Consider using
==instead ofcmp(...) == Ordering::Equalfor address comparison- The error only includes the address recovered with v=0, but it might be helpful to also include the attempt with v=1
let sig0_address = sig.recover(message)?; - if sig0_address.cmp(&expected_address) == Ordering::Equal { + if sig0_address == expected_address { return Ok(sig); } sig.v = 1; let sig1_address = sig.recover(message)?; - if sig1_address.cmp(&expected_address) == Ordering::Equal { + if sig1_address == expected_address { return Ok(sig); } Err(ethers::types::SignatureError::VerificationError( expected_address, sig0_address, ))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
orchestrator/gravity/src/ethereum/types.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: rust-test
- GitHub Check: rust-build
- GitHub Check: hardhat-build
- GitHub Check: Clippy
- GitHub Check: node-build (16.x)
- GitHub Check: local-tests (16.x)
- GitHub Check: go-build
🔇 Additional comments (3)
orchestrator/gravity/src/ethereum/types.rs (3)
6-6: Clean import addition for address comparison.The import for
Orderingfromstd::cmpis appropriate for the new address comparison logic.
26-50: Excellent handling of GCP KMS signature normalization.The implementation correctly addresses the issue where GCP KMS doesn't produce signatures normalized for EVM. The method systematically:
- Tries with v=0
- If that fails, tries with v=1
- Returns an appropriate error if both fail
This is a robust solution to normalize the ECDSA recovery ID for Ethereum transactions.
51-53: Good default case handling.The default case correctly handles other signer types by simply cloning the signature without modification, which is appropriate since the comment indicates that LocalWallet doesn't need v correction.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
orchestrator/gravity/src/ethereum/types.rs (2)
6-6: Consider using direct equality checks instead ofOrdering.Since we only need to verify equality (rather than a detailed ordering), a direct
if a == b { ... }comparison is more idiomatic and slightly clearer than usingcmp(...) == Ordering::Equal.
18-55: Overall Implementation Looks Good, But Simplify Equality Checks and Error Messaging.
Use direct equality checks
For lines 34 and 42, consider:-if sig0_address.cmp(&expected_address) == Ordering::Equal { +if sig0_address == expected_address {This approach is more explicit and avoids confusion about partial ordering semantics.
Enrich verification error
Since this logic attempts twovvalues, it might be helpful to clarify in the error message which attempts failed. For instance, logging thatv=0andv=1both yielded addresses that differ from the expected address can assist in debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
orchestrator/gravity/src/ethereum/types.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: rust-test
- GitHub Check: rust-build
- GitHub Check: hardhat-build
- GitHub Check: node-build (16.x)
- GitHub Check: go-build
- GitHub Check: local-tests (16.x)
- GitHub Check: Clippy
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
orchestrator/gravity/src/ethereum/types.rs (2)
18-72: Consider more direct equality checks when verifying the recovered address.
Usingif recovered_address == expected_addresscan be more straightforward thancmp(...), improving clarity.You could replace:
if sig0_address.cmp(&expected_address) == Ordering::Equal { return Ok(sig); }with:
if sig0_address == expected_address { return Ok(sig); }
83-87: Simplify the message copy step.
You can directly convert the message reference to aVec<u8>using.to_vec(), making the code more concise.- let mut msg = vec![0u8; message.as_ref().len()]; - msg.copy_from_slice(message.as_ref()); + let msg = message.as_ref().to_vec();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
orchestrator/gravity/src/ethereum/types.rs(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
orchestrator/gravity/src/ethereum/types.rs (1)
orchestrator/gravity/src/utils/error.rs (16)
from(124-126)from(130-132)from(136-138)from(142-144)from(148-150)from(154-156)from(160-162)from(166-168)from(172-174)from(178-180)from(184-186)from(190-192)from(196-198)from(202-204)from(208-210)from(214-216)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: hardhat-build
- GitHub Check: go-build
- GitHub Check: rust-test
- GitHub Check: rust-build
- GitHub Check: Clippy
- GitHub Check: local-tests (16.x)
- GitHub Check: node-build (16.x)
🔇 Additional comments (3)
orchestrator/gravity/src/ethereum/types.rs (3)
6-6: Import change looks good.
The addition ofOrderingis aligned with the usage ofcmpin signature comparison.
88-97: Good approach to handle signature creation for both local and GCP KMS signers.
No immediate issues found; the match statement and unified error handling are clear.
99-99:Details
❓ Verification inconclusive
Request confirmation regarding any internal prefixing done by GcpKms.
If the GcpKms signer applies a message prefix (e.g., Ethereum’s “\x19Ethereum Signed Message:”), the recovered address might differ. Please confirm thatsign_messageuses the exact same bytes.
🏁 Script executed:
#!/bin/bash # Description: Search for potential prefix usage in `ethers_gcp_kms_signer` that might affect recovery. fd '.*\.rs' | xargs rg 'Ethereum Signed Message'Length of output: 50
Action Required: Confirm GcpKms Signer Does Not Implicitly Prefix Messages
Our automated search for the string
"Ethereum Signed Message"produced no results, which suggests that there is no obvious internal prefixing within the code. However, the lack of output prevents us from making a definitive conclusion. Please manually verify that the GcpKms signer’s implementation ofsign_messagedoes not apply any internal message prefixing (such as the Ethereum prefix) so that the bytes signed match exactly the ones used in the address recovery process.
- Location to Check:
orchestrator/gravity/src/ethereum/types.rs(line 99:self.normalize(&msg, &sig))- The GcpKms signer implementation (likely within the ethers_gcp_kms_signer module)
This reverts commit 5eb3886.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
orchestrator/gravity/src/ethereum/types.rs (1)
68-72: Use a direct vector conversion.
Instead of creating a zeroed vector and copying the slice, you could do:- let mut msg = vec![0u8; message.as_ref().len()]; - - msg.copy_from_slice(message.as_ref()); + let msg = message.as_ref().to_vec();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
orchestrator/gravity/src/ethereum/types.rs(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: solidity-build
- GitHub Check: rust-build
- GitHub Check: rust-test
- GitHub Check: hardhat-build
- GitHub Check: go-build
- GitHub Check: Clippy
- GitHub Check: node-build (16.x)
- GitHub Check: local-tests (16.x)
🔇 Additional comments (9)
orchestrator/gravity/src/ethereum/types.rs (9)
6-6: Imports look good.
No issues here;cmp::OrderingandArcare both used properly.
18-24: Neat separation of normalization logic.
Definingnormalizeas a dedicated method makes the functionality clear and maintainable.
25-56: Consider adding targeted unit tests.
This logic correctly retries withv=0and thenv=1, but including explicit test cases for both normalized signatures would help ensure ongoing correctness.Would you like a quick snippet that demonstrates how to test both paths?
73-82: Implementation looks good.
Mapping the error intoProviderError::CustomErroris consistent for both signers.
84-84: Good addition of normalization.
Ensures the signature is adjusted for the Gravity system as soon as it’s produced.
88-88: Transaction signing flow is consistent.
Nicely mirrors the logic fromsign_messagewhile handling local vs. KMS signers.
98-101: Hash-based recovery is handled correctly.
Retrievingsighashbefore callingnormalizeis a sound approach for transaction-level verification.
129-139: Typed data flow aligns with EIP-712.
Matches the pattern used for the other sign methods, returning consistent error handling.
140-143:Details
❓ Verification inconclusive
Validate EIP-712 hashing.
Confirm that this follows the correct EIP-712 domain separator and data struct approach across your codebase.Below is a script to search for additional EIP-712 usage and confirm consistent hashing:
🏁 Script executed:
#!/bin/bash # Searching for other references to encode_eip712 fd -e rs | xargs rg "encode_eip712"Length of output: 121
EIP-712 Hashing Implementation Verification
The only occurrence of
encode_eip712in the repository is inorchestrator/gravity/src/ethereum/types.rs(lines 140–143). Please ensure that the implementation:
- Correctly constructs the EIP‑712 domain separator and data structure as per the standard.
- Returns a hash consistent with the specification.
- Appropriately maps errors to
ethers::providers::ProviderError::CustomError.Manual verification of these aspects is advised to confirm consistent behavior across all intended use cases.
2 participants
Summary by CodeRabbit
Bug Fixes
New Features