refactor(e2e): load EVM addresses dynamically#3512
Conversation
gartnera
added
no-changelog
|
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 📝 WalkthroughWalkthroughThis pull request refactors the CLI token management and balance retrieval logic. It introduces functions to register and process ERC20 and ZRC20 flags, including new error handling and context cancellation in command execution. The contract setup logic is modularized with functions to retrieve chain parameters and foreign coin data. In addition, an interface for ERC20 balance retrieval is defined, replacing previous implementations to improve flexibility in balance fetching. Redundant flag declarations and functions have been removed to streamline the processing of token configurations. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant BC as BalancesCmd
participant REF as registerERC20Flags
participant PZF as processZRC20Flags
participant Ctx as Context (defer cancel)
U->>BC: Execute balance command
BC->>REF: Register ERC20 flags
BC->>PZF: Process ZRC20/ERC20 flags
PZF-->>BC: Return processed flag values (or error)
BC->>Ctx: Call defer cancel(nil)
sequenceDiagram
participant Caller as Balance Caller
participant G as getERC20BalanceSafe
participant ERC as ERC20BalanceOf Interface
Caller->>G: Request ERC20 balance for account
alt Interface is not nil
G->>ERC: Invoke BalanceOf for balance retrieval
ERC-->>G: Return balance value
else Interface is nil
G-->>G: Log issue and create new big.Int
end
G-->>Caller: Return finalized balance value
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
|
gartnera
force-pushed
the
e2e-dynamic-addresses
branch
from
5e97b07 to
ceb03d3
Compare
gartnera
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3512 +/- ##
===========================================
- Coverage 65.54% 65.52% -0.03%
===========================================
Files 444 445 +1
Lines 30563 30606 +43
===========================================
+ Hits 20034 20055 +21
- Misses 9675 9697 +22
Partials 854 854 |
gartnera
force-pushed
the
e2e-dynamic-addresses
branch
from
8f24316 to
e4c6b35
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
cmd/zetae2e/config/contracts.go (3)
33-43: Consider returning an error when the selector fails to match.
Currently, this function returnsnilif no matchingChainParamsis found, which may lead to unhandlednilreferences downstream. Introducing a clear error message in such a case can make debugging simpler.func chainParamsBySelector( chainParams []*types.ChainParams, selector func(chainID int64, additionalChains []chains.Chain) bool, ) (*types.ChainParams, error) { for _, chainParam := range chainParams { if selector(chainParam.ChainId, nil) { - return chainParam + return chainParam, nil } } - return nil + return nil, fmt.Errorf("no chain parameters matched the selector") }
45-52: Optional enhancement for chainParamsByChainID.
Similar to the previous function, returning an error rather thannilcould improve the robustness of error handling.
104-105: Direct assertion may be too strict for non-test usage.
Callingrequire.NoError(r, err)aborts on error, which is typically used in tests rather than production-like code. If this function is partly used outside of test contexts, consider returning the error to handle it gracefully.cmd/zetae2e/balances.go (1)
52-57: Consider providing an explicit error cause tocancel.
Callingdefer cancel(nil)will remove the context without a specific error cause. If you prefer capturing errors in cancellation, you might pass the actual error object for better traceability. Otherwise, this usage is acceptable.cmd/zetae2e/common.go (2)
25-53: Flag processing logic is correct.
When both network and symbol are specified, the code retrieves the relevant addresses. However, if multiple coins share a prefix or suffix, there may be ambiguity in matching the symbol. If that becomes a frequent scenario, consider refining the symbol-matching logic.
55-103: ZRC20 discovery is neatly structured.
The partial symbol match (prefix/suffix) could cause collisions if multiple tokens share naming similarities. If this is expected, keep it as is; otherwise, consider validating exact matches or providing a more robust lookup mechanism.e2e/runner/balances.go (1)
45-57: Consider a more descriptive function name.While the implementation is solid, the function name could better reflect its safe nature and nil-checking behavior.
Consider renaming to
getSafeERC20BalanceorgetERC20BalanceWithNilCheck:-func (r *E2ERunner) getERC20BalanceSafe(z ERC20BalanceOf, name string) *big.Int { +func (r *E2ERunner) getSafeERC20Balance(z ERC20BalanceOf, name string) *big.Int {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/zetae2e/balances.go(3 hunks)cmd/zetae2e/common.go(1 hunks)cmd/zetae2e/config/contracts.go(3 hunks)cmd/zetae2e/run.go(2 hunks)e2e/runner/balances.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/balances.gocmd/zetae2e/common.goe2e/runner/balances.gocmd/zetae2e/run.gocmd/zetae2e/config/contracts.go
🔇 Additional comments (17)
cmd/zetae2e/config/contracts.go (6)
6-6: No issues with new imports.
These imports are standard in Go for Ethereum-related functionalities and test assertions.Also applies to: 8-8
24-25: Imports confirmed.
These are valid imports for working with chain data and fungible types. No concerns here.Also applies to: 29-30
107-114: Approach for Solana chain parameters looks consistent.
No particular concerns; usage ofchainParamsBySelectoris straightforward, and the code sets the Solana gateway program only if the config or chain parameters are present.
141-144: Fetching foreign coins logic is valid.
The error handling and usage ofFungible.ForeignCoinsAllfor retrieving external coin data appear consistent with the rest of the code.
149-157: Referenced pointer inforeignCoinByChainIDreappears here.
This usage depends on the pointer fix recommended above. Once that is resolved, referencingethForeignCoinhere should be safe.
158-166: Initialization of SystemContract is correct.
The address validation and instantiation pattern aligns with the rest of the codebase. No additional issues.cmd/zetae2e/balances.go (2)
5-5: Import statement approved.
Including"fmt"is necessary for formatted error wrapping.
31-31: ERC20 flags registration approved.
Registering ERC20 flags here consolidates flag declarations, improving maintainability.cmd/zetae2e/common.go (4)
1-2: New package creation is acceptable.
Defining this aspackage mainis a reasonable approach for a CLI tool.
3-16: Imports appear relevant and valid.
This set of imports covers the entire functionality needed for chain-specific calls, string utilities, and CLI interactions.
17-18: Flag constants declared properly.
Straightforward usage of constants for CLI flags.
20-23: Registration of ERC20 flags is concise.
The function straightforwardly attaches CLI flags for future processing.cmd/zetae2e/run.go (3)
45-45: LGTM: Flag registration is properly placed.The
registerERC20Flagscall is well-positioned before other flag registrations, maintaining a clean separation of concerns.
73-76: LGTM: Error handling follows best practices.The error handling is idiomatic Go, with proper error wrapping and descriptive messages.
79-80: LGTM: Context handling is robust.Using
context.WithCancelCauseis a good practice for propagating cancellation reasons, and the deferred cleanup ensures proper resource management.e2e/runner/balances.go (2)
41-43: LGTM: Interface is well-defined and focused.The
ERC20BalanceOfinterface follows Go's interface naming conventions and uses appropriate types from go-ethereum.
70-74: LGTM: Balance retrieval is consistent and robust.The balance retrieval implementation consistently uses the safe balance checking method across all token types.
3 participants
Pull all EVM contract addresses from the gateway attributes rather than looking at the config.Can't do this since thezetaConnectorattribute is 0Pull all EVM contract addresses from the observer chain parameters. Make balance checks a bit more safe so that they won't make infrastructure CI hard fail.
This feels a bit messy overall. The ultimate goal is to completely purge contract addresses from the config. This intermediate state is a bit awkward.
Summary by CodeRabbit
New Features
Refactor