fix: SetGatewayAddress might set invalid gateway address #3030
Conversation
ws4charlie
requested review from
brewmaster012,
kingpinXD,
lumtis,
skosito,
swift1337 and
fbac
as code owners
📝 WalkthroughWalkthroughThe pull request includes extensive updates to the ZetaChain node, emphasizing new features, refactoring, testing enhancements, and various fixes. Key features are the introduction of stateful precompiled contracts, a common RPC package, and improvements to Bitcoin and staking functionalities. Refactoring efforts focus on code quality, including lint fixes and unused code removal. Testing coverage has been expanded, particularly for stateful contracts and Bitcoin functionalities. Additionally, several issues have been addressed, improving overall functionality and robustness. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3030 +/- ##
===========================================
+ Coverage 64.06% 64.12% +0.05%
===========================================
Files 412 412
Lines 28960 28961 +1
===========================================
+ Hits 18554 18570 +16
+ Misses 9611 9596 -15
Partials 795 795
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
zetaclient/chains/solana/signer/signer_test.go (1)
119-137: Consider adding more edge cases to the test suite.While the current test cases cover basic scenarios, consider adding tests for:
- Empty string gateway address
- Gateway address with leading/trailing whitespace
- Address with different encoding formats
- Maximum length address validation
zetaclient/chains/solana/signer/signer.go (1)
195-196: LGTM! Consider enhancing error observability.The early return prevents invalid gateway address state, which is the core fix needed.
Consider enhancing the error logging with structured fields for better observability:
-signer.Logger().Std.Error().Err(err).Msgf("cannot parse gateway address: %s", address) +signer.Logger().Std.Error(). + Err(err). + Str("address", address). + Str("chain", signer.Chain().Name). + Msg("cannot parse gateway address")changelog.md (1)
59-59: Consider enhancing the changelog entry with more context.While the entry correctly documents the fix, consider expanding it to provide more context about the potential impact and importance of this change. This helps users understand the significance of the fix.
Consider this enhanced version:
-* [3030](https://github.com/zeta-chain/node/pull/3030) - Avoid storing invalid Solana gateway address in the `SetGatewayAddress` +* [3030](https://github.com/zeta-chain/node/pull/3030) - Fix validation in `SetGatewayAddress` to prevent storing invalid Solana gateway addresses, ensuring proper address validation and system stability
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- changelog.md (1 hunks)
- zetaclient/chains/solana/signer/signer.go (1 hunks)
- zetaclient/chains/solana/signer/signer_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
zetaclient/chains/solana/signer/signer.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/solana/signer/signer_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (2)
zetaclient/chains/solana/signer/signer.go (2)
Line range hint
223-224: LGTM! Efficient guard clause implementation.The early return using HasRelayerKey() check prevents unnecessary RPC calls, improving efficiency.
Line range hint
1-275: Consider reviewing concurrent access patterns.The Signer struct uses mutex locks in some methods (SetGatewayAddress, GetGatewayAddress) but not in others (SetRelayerBalanceMetrics). While the current implementation might be safe due to initialization patterns, it would be beneficial to document the concurrent access assumptions and requirements.
Let's verify the concurrent access patterns:
✅ Verification successful
Let me gather more information about the concurrent access patterns in the Solana signer implementation.
Let me try a different approach to analyze the concurrent access patterns.
Based on the analysis of the concurrent access patterns, I can now provide a final response.
Concurrent access patterns are correctly implemented with room for documentation improvements
The concurrent access implementation in the Solana signer is well-structured:
- Mutex protection is correctly applied to shared state modifications (gatewayID, pda) through
SetGatewayAddressandGetGatewayAddressSetRelayerBalanceMetricsdoesn't require mutex protection as it only reads the immutablerelayerKeyfield, which is set during initialization- The base Signer struct provides the mutex and locking mechanisms inherited by all chain-specific implementations
- Critical state changes are properly synchronized using the Lock/Unlock pattern
Suggestions for improvement:
- Add documentation comments explaining the thread-safety guarantees for each field in the Signer struct
- Consider documenting which methods are safe for concurrent access and which are not
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for mutex usage patterns in signer implementations rg -A 2 "Lock\(\)" zetaclient/chains/Length of output: 6757
Script:
#!/bin/bash # Search for methods that access shared state in Solana signer ast-grep --pattern 'func \(signer \*Signer\) $_($$$) $_ { $$$ }'Length of output: 68
Script:
#!/bin/bash # Search for field accesses in Solana signer methods rg -A 2 "signer\." zetaclient/chains/solana/signer/signer.go # Also check the base signer implementation rg -A 2 "type Signer struct" zetaclient/chains/base/signer.goLength of output: 2599
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Description
Fixed the issue in method
SetGatewayAddressand add unit test.How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Tests