fix(zetaclient): orchestrator: signer resolution #3139
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes updates to the changelog, reflecting various enhancements, tests, refactoring, and fixes across the codebase. Key changes include the introduction of a Whitelist message capability for SPL tokens, improvements to build reproducibility, and the integration of SPL deposits. The logging mechanisms in the Changes
Assessment against linked issues
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
|
swift1337
force-pushed
the
fix/orchestrator-logging
branch
from
98d4030 to
c1c1404
Compare
swift1337
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
skosito and
fbac
as code owners
swift1337
added
UPGRADE_LIGHT_TESTS
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
zetaclient/chains/solana/signer/signer.go (3)
283-286: Optimize lock scope for better concurrencyThe lock scope can be reduced to only wrap the critical section where shared state is modified.
- signer.Lock() - signer.gatewayID = gatewayID - signer.pda = pda - signer.Unlock() + func() { + signer.Lock() + defer signer.Unlock() + signer.gatewayID = gatewayID + signer.pda = pda + }()
269-271: Enhance error logging structureConsider using structured logging fields for better error tracking and analysis.
- signer.Logger().Std.Error().Err(err).Str("address", address).Msgf("Unable to parse gateway address") + signer.Logger().Std.Error(). + Err(err). + Str("address", address). + Str("function", "SetGatewayAddress"). + Msg("Unable to parse gateway address")
Refactoring needed: EVM-specific functions should be removed from interface
The verification reveals a deeper architectural issue:
- The
ChainSignerinterface inzetaclient/chains/interfaces/interfaces.gorequires all chain implementations (EVM, Bitcoin, Solana, TON) to implement EVM-specific methods- These methods are only meaningfully used in EVM chains, as evidenced by empty implementations in Bitcoin, Solana, and TON signers
- The orchestrator (
zetaclient/orchestrator/orchestrator.go) only calls these methods for EVM chainsRecommended actions:
- Extract EVM-specific methods into a separate
EVMChainSignerinterface- Update the
ChainSignerinterface to remove these methods- Modify the orchestrator to use type assertion for EVM-specific operations
🔗 Analysis chain
Line range hint
313-332: Verify the timeline for removing EVM-specific functionsThese EVM-specific functions in the Solana signer violate interface segregation principle. While they are properly tracked for removal in issue #2532, let's verify the removal plan.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the status of issue #2532 and find any dependent code gh issue view 2532 # Find any usage of these functions in the codebase rg -A 2 "SetZetaConnectorAddress|SetERC20CustodyAddress|GetZetaConnectorAddress|GetERC20CustodyAddress" \ --type go \ --glob '!zetaclient/chains/solana/signer/signer.go'Length of output: 8041
zetaclient/chains/ton/signer/signer.go (2)
281-281: LGTM! Consider enhancing error logging.The guard clause correctly prevents unnecessary updates when the address is empty or unchanged. This aligns with the PR objectives and maintains thread safety.
Consider adding debug logging for skipped updates to aid in troubleshooting:
if addr == "" || s.gateway.AccountID().ToRaw() == addr { + s.Logger().Std.Debug(). + Str("addr", addr). + Str("current_addr", s.gateway.AccountID().ToRaw()). + Msg("Skipping gateway address update") return }
291-295: LGTM! Consider enhancing log message clarity.The structured logging implementation provides excellent traceability by capturing both old and new addresses.
Consider making the log message more specific about the component being updated:
s.Logger().Std.Info(). Str("signer.old_gateway_address", s.gateway.AccountID().ToRaw()). Str("signer.new_gateway_address", acc.ToRaw()). - Msg("Updated gateway address") + Msg("TON signer gateway address updated")zetaclient/chains/evm/signer/signer.go (3)
111-114: Improve comment clarity for early return conditions.The comment "noop" doesn't effectively describe the conditions for early return. Consider being more explicit about the validation checks.
- // noop + // Skip if the address is zero or unchanged
128-131: Improve comment clarity for early return conditions.The comment "noop" doesn't effectively describe the conditions for early return. Consider being more explicit about the validation checks.
- // noop + // Skip if the address is zero or unchanged
147-150: Improve comment clarity for early return conditions.The comment "noop" doesn't effectively describe the conditions for early return. Consider being more explicit about the validation checks.
- // noop + // Skip if the address is zero or unchangedchangelog.md (1)
23-23: Consider enhancing the changelog entry with more details.The current entry "fix config resolution in orchestrator" could be more descriptive. Consider adding details about:
- What specific configuration issue was fixed
- The impact of the fix on the orchestrator's behavior
- Any notable changes in configuration handling
Example:
-* [3139](https://github.com/zeta-chain/node/pull/3139) - fix config resolution in orchestrator +* [3139](https://github.com/zeta-chain/node/pull/3139) - fix signer address resolution in orchestrator to ensure proper address validation and comparison
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
changelog.md(1 hunks)zetaclient/chains/evm/signer/signer.go(1 hunks)zetaclient/chains/solana/signer/signer.go(1 hunks)zetaclient/chains/ton/signer/signer.go(2 hunks)zetaclient/orchestrator/orchestrator.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
zetaclient/chains/evm/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.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/ton/signer/signer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/orchestrator/orchestrator.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (4)
zetaclient/chains/ton/signer/signer.go (1)
Line range hint 281-297: Consider potential race condition in address comparison.
There's a potential race condition between checking the current gateway address and updating it. While the current implementation is functionally correct due to the idempotent nature of the operation, consider moving the comparison inside the lock for strict thread safety.
Consider this alternative implementation:
func (s *Signer) SetGatewayAddress(addr string) {
- if addr == "" || s.gateway.AccountID().ToRaw() == addr {
+ if addr == "" {
+ s.Logger().Std.Debug().Str("addr", addr).Msg("Skipping empty gateway address update")
return
}
acc, err := ton.ParseAccountID(addr)
if err != nil {
s.Logger().Std.Error().Err(err).Str("addr", addr).Msg("unable to parse gateway address")
return
}
s.Lock()
+ if s.gateway.AccountID().ToRaw() == addr {
+ s.Logger().Std.Debug().Str("addr", addr).Msg("Skipping unchanged gateway address")
+ s.Unlock()
+ return
+ }
+
s.Logger().Std.Info().
Str("signer.old_gateway_address", s.gateway.AccountID().ToRaw()).
Str("signer.new_gateway_address", acc.ToRaw()).
- Msg("Updated gateway address")
+ Msg("TON signer gateway address updated")
- s.Lock()
s.gateway = toncontracts.NewGateway(acc)
s.Unlock()
}Let's verify the thread safety of this method:
✅ Verification successful
Let me gather more information about concurrent access patterns to the gateway field.
Race condition concern is valid but impact is minimal due to design.
The code analysis reveals that while there is a theoretical race condition between the address comparison and update, the practical impact is minimal because:
- The gateway field is protected by mutex in critical sections
- The operation is idempotent - multiple identical updates result in the same final state
- The gateway is primarily read in
GetGatewayAddress()andProcessOutbound(), both properly synchronized
However, the suggested refactoring would indeed provide stricter thread safety and better logging. The proposed changes are production-grade improvements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for concurrent access patterns to gateway address
# Look for other goroutines or concurrent operations that might access the gateway address
# Search for gateway address access patterns
rg -A 5 "gateway.*ToRaw\(\)" --type go
# Search for concurrent patterns (goroutines, channels) near gateway usage
rg -B 5 -A 5 "go\s+func|select|chan" --type go | rg -B 5 -A 5 "gateway"
Length of output: 6908
Script:
#!/bin/bash
# Check for all methods accessing the gateway field
ast-grep --pattern 's.gateway'
# Check for any concurrent operations in the Signer type
rg -B 10 -A 10 'type Signer struct' zetaclient/chains/ton/signer/signer.go
# Look for other methods that might use the gateway concurrently
rg -l 'func.*Signer.*\{' zetaclient/chains/ton/signer/signer.go | xargs cat
Length of output: 10283
zetaclient/orchestrator/orchestrator.go (2)
12-12: LGTM: Import alias simplification
The simplified import alias improves readability while maintaining the same functionality.
171-171: Consider validating gateway addresses for Solana and TON
The gateway address setting for Solana and TON chains lacks validation. Consider adding chain-specific address validation to prevent issues with invalid addresses.
Let's verify the address validation in the respective chain implementations:
Also applies to: 173-173
changelog.md (1)
Line range hint 1-1000: LGTM! Well-structured changelog.
The changelog follows best practices with:
- Clear version sections
- Consistent categorization of changes
- Proper PR linking
- Reverse chronological ordering
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3139 +/- ##
===========================================
- Coverage 63.15% 63.08% -0.07%
===========================================
Files 423 423
Lines 29872 29875 +3
===========================================
- Hits 18865 18848 -17
- Misses 10169 10186 +17
- Partials 838 841 +3
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
swift1337
removed
TON_TESTS
Closes #3096
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
SetGatewayAddressmethods across various chains.