fix: support tracker processing for v2 inbounds #3179
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📝 WalkthroughWalkthroughThis pull request introduces multiple enhancements and fixes across various components of the ZetaChain node. Key updates include the ability to whitelist SPL tokens on Solana, improvements to transaction handling, and the introduction of inbound trackers for v2 CCTX. Configuration files have been updated to reflect new contract addresses, and several methods have been added or modified to support these features. The changelog has been comprehensively updated to document these changes, ensuring clarity on the evolution of the codebase. Changes
Possibly related 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3179 +/- ##
===========================================
- Coverage 62.85% 62.70% -0.15%
===========================================
Files 424 425 +1
Lines 30138 30209 +71
===========================================
Hits 18942 18942
- Misses 10358 10429 +71
Partials 838 838
|
lumtis
requested review from
brewmaster012,
kingpinXD,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (6)
e2e/e2etests/test_inbound_trackers.go (1)
24-29: Enhance helper function robustness and documentation.The helper function could be improved for better error handling and test flow control:
-addTrackerAndWaitForCCTX := func(coinType coin.CoinType, txHash string) { +// addTrackerAndWaitForCCTX adds an inbound tracker and waits for the CCTX to be mined +// returns the mined CCTX or errors if timeout occurs +addTrackerAndWaitForCCTX := func(coinType coin.CoinType, txHash string) *crosschaintypes.CrossChainTx { r.AddInboundTracker(coinType, txHash) cctx := utils.WaitCctxMinedByInboundHash(r.Ctx, txHash, r.CctxClient, r.Logger, r.CctxTimeout) require.EqualValues(r, crosschaintypes.CctxStatus_OutboundMined, cctx.CctxStatus.Status) r.Logger.CCTX(*cctx, "cctx") + return cctx }e2e/e2etests/e2etests.go (1)
741-746: Consider enhancing the test definition with more details and arguments.The test definition could benefit from:
- A more descriptive test description that explains what aspects of inbound tracker processing are being tested
- Common test arguments like amounts, addresses, or other configuration parameters that might be needed for thorough testing
Consider updating the test definition:
runner.NewE2ETest( TestInboundTrackersName, - "test processing inbound trackers for observation", + "test processing of v2 inbound trackers for deposit, depositAndCall, and call operations", []runner.ArgDefinition{ + {Description: "deposit amount", DefaultValue: "1000000000000000000"}, + {Description: "gas limit", DefaultValue: "300000"}, }, TestInboundTrackers, ),changelog.md (3)
35-35: Expand the v2 inbound tracker changelog entry.The entry "support inbound trackers for v2 cctx" should be expanded to provide more context about the changes and impact, especially since this is a key feature mentioned in the PR objectives.
Consider expanding it to:
-* [3179](https://github.com/zeta-chain/node/pull/3179) - support inbound trackers for v2 cctx +* [3179](https://github.com/zeta-chain/node/pull/3179) - support inbound trackers for v2 CCTX, enabling processing of deposit, depositAndCall, and call operations for observer v2
27-34: Add missing PR/issue references.Some fixes lack corresponding PR/issue references, which makes it harder to track the context and discussions behind these changes.
Add PR/issue references for entries like:
- "register messages for emissions module to legacy amino codec"
- "fix potential panic in the Bitcoin inscription parsing"
Line range hint
6-35: Improve changelog structure for better readability.The changelog structure could be improved to make it easier to scan and understand the changes.
Consider:
- Using consistent punctuation at the end of each entry
- Grouping related changes together (e.g., all Bitcoin-related fixes)
- Adding a brief summary at the start of each version highlighting the major changes
zetaclient/chains/evm/observer/inbound.go (1)
171-173: Enhance error handling to continue processing remaining trackersCurrently, an error with one inbound tracker halts the entire processing loop due to the
returnstatement. This could result in unprocessed trackers if an error occurs. Modify the error handling to log the error and continue processing subsequent trackers to ensure that all trackers are evaluated.Apply this diff to adjust the error handling logic:
if err != nil { - return errors.Wrapf(err, "error checking and voting for inbound %s chain %d", tx.Hash, ob.Chain().ChainId) + ob.Logger().Inbound.Err(err).Msgf("error checking and voting for inbound %s chain %d", tx.Hash, ob.Chain().ChainId) + continue }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
changelog.md(2 hunks)cmd/zetae2e/config/local.yml(1 hunks)cmd/zetae2e/run.go(3 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_inbound_trackers.go(1 hunks)e2e/runner/zeta.go(2 hunks)zetaclient/chains/base/observer.go(1 hunks)zetaclient/chains/evm/observer/inbound.go(3 hunks)zetaclient/chains/evm/observer/v2_tracker.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- zetaclient/chains/base/observer.go
🧰 Additional context used
📓 Path-based instructions (6)
cmd/zetae2e/run.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/e2etests.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_inbound_trackers.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/zeta.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/v2_tracker.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (7)
e2e/e2etests/test_inbound_trackers.go (3)
1-13: LGTM: Well-structured imports and package declaration.
The imports are properly organized and all are being utilized in the code.
31-87: 🛠️ Refactor suggestion
Refactor test scenarios for better maintainability.
The test scenarios could be improved in several ways:
- Use table-driven tests to reduce code duplication
- Define constants for magic numbers (e.g.,
1e17,0) - Add more descriptive error messages in assertions
Here's a suggested refactor:
type testCase struct {
name string
coinType coin.CoinType
amount *big.Int
execute func() string // returns tx hash
}
const (
defaultAmount = 1e17
zeroGasLimit = 0
)
func TestInboundTrackers(r *runner.E2ERunner, args []string) {
require.Len(r, args, 0)
amount := big.NewInt(defaultAmount)
cases := []testCase{
{
name: "v1 eth deposit",
coinType: coin.CoinType_Gas,
amount: amount,
execute: func() string {
return r.DepositEtherWithAmount(amount).Hex()
},
},
// Add other test cases...
}
for _, tc := range cases {
r.Logger.Printf("🏃test %s", tc.name)
txHash := tc.execute()
addTrackerAndWaitForCCTX(tc.coinType, txHash)
r.Logger.Printf("🍾%s observed", tc.name)
}
}This refactor:
- Reduces code duplication
- Makes it easier to add new test cases
- Improves maintainability
- Makes the test structure more clear
Let's verify the usage of magic numbers in the codebase:
15-23: Improve test setup and documentation reliability.
The current implementation has several concerns:
- Referencing specific line numbers in comments is fragile as they can change with code modifications
- Requiring source code modification to run tests is a significant code smell
- The test setup could be more robust with proper test configuration
Consider these improvements:
- Replace the line number reference with a more stable identifier (e.g., function name or unique comment)
- Implement a proper test configuration mechanism to disable inbound observation
- Add validation to ensure the test environment is properly configured
Let's verify the referenced line and its surrounding context:
cmd/zetae2e/config/local.yml (2)
117-117:
Verify custody contract address update
The custody contract address has been updated. This is a critical contract for handling deposits, and we need to ensure this change is intentional and the new contract is properly deployed.
113-113: Verify test_dapp_v2 contract addresses
The addition of test_dapp_v2 contracts aligns with the PR objective to support v2 inbound processing. However, let's verify these addresses are correctly deployed and consistent across environments.
Also applies to: 121-121
✅ Verification successful
Contract addresses appear to be correctly configured
The test_dapp_v2 contract addresses are properly configured and consistent with the codebase:
- The addresses are referenced in the config files and test data
- Address
0xBFF76e77D56B3C1202107f059425D56f0AEF87Edis used in both ZEVM config and test fixtures - Address
0xa825eAa55b497AF892faca73a3797046C10B7c23is properly configured as the EVM test_dapp_v2 - The config structure is properly defined in
e2e/config/config.gowith the corresponding yaml tags
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test_dapp_v2 contract addresses exist and are consistent
# Check if the contracts exist in both networks
echo "Checking ZEVM contract..."
curl -s -X POST -H "Content-Type: application/json" --data '{"jsonrpc":"2.0","method":"eth_getCode","params":["0xBFF76e77D56B3C1202107f059425D56f0AEF87Ed", "latest"],"id":1}' http://localhost:9545
echo "Checking EVM contract..."
curl -s -X POST -H "Content-Type: application/json" --data '{"jsonrpc":"2.0","method":"eth_getCode","params":["0xa825eAa55b497AF892faca73a3797046C10B7c23", "latest"],"id":1}' http://localhost:8545
Length of output: 576
Script:
#!/bin/bash
# Let's try a different approach to verify the contract addresses
# Search for test_dapp_v2 references in the codebase
echo "Searching for test_dapp_v2 references..."
rg "test_dapp_v2" -A 2 -B 2
# Check if these addresses are used in test files
echo -e "\nChecking test files for contract addresses..."
rg "0xBFF76e77D56B3C1202107f059425D56f0AEF87Ed|0xa825eAa55b497AF892faca73a3797046C10B7c23" -A 2 -B 2
# Look for contract deployment or setup files
echo -e "\nSearching for contract deployment files..."
fd -t f "deploy|setup|init" --exec grep -l "test_dapp_v2"
Length of output: 4603
e2e/runner/zeta.go (2)
18-18: LGTM: Import statement follows Go conventions.
The addition of the coin package import is necessary for the new functionality and follows proper organization.
344-346: Consider making the policy name configurable.
The emergency policy name is hardcoded, which could limit flexibility in testing different scenarios. Consider making this configurable through the E2E runner's configuration.
Let's verify the usage of this emergency policy name across the codebase:
How is this related to EVM inbounds V2? |
It's failing right now if you try to run |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
| return errors.Wrapf(err, "error checking and voting for inbound %s chain %d", tx.Hash, ob.Chain().ChainId) | ||
| ob.Logger().Inbound.Debug().Err(err).Msg("error getting gateway contract for processing inbound tracker") | ||
| } | ||
| if err == nil && tx != nil && ethcommon.HexToAddress(tx.To) == gatewayAddr { |
There was a problem hiding this comment.
Nit
The tx != nil check should, IMO, be moved up to line 127 where TransactionByHash() is called or removed at all. The reason being, that if tx == nil, the else branch is entered
node/zetaclient/chains/evm/observer/inbound.go
Lines 157 to 177 in 85d508a
which internally also accesses tx. For example, in
There was a problem hiding this comment.
Makes sense, the idea to touch the other v1 logic as less as possible so not having the check in this workflow
| } | ||
| msg := ob.newDepositInboundVote(eventDeposit) | ||
| _, err = ob.PostVoteInbound(ctx, &msg, zetacore.PostVoteInboundExecutionGasLimit) | ||
| return err |
There was a problem hiding this comment.
As soon as an event is processed and posted (i.e., lines 48, 65, 82), it returns without processing the remaining receipt.Logs. Given the current implementation of GatewayEVM, it seems not possible to have multiple gateway events within the same tx, so it's not really a big deal.
Maybe consider processing all receipt.Logs?
There was a problem hiding this comment.
Given the current implementation of GatewayEVM, it seems not possible to have multiple gateway events within the same tx, so it's not really a big deal.
This is the reason yes
| return errors.Wrapf(err, "error checking and voting for inbound %s chain %d", tx.Hash, ob.Chain().ChainId) | ||
| ob.Logger().Inbound.Debug().Err(err).Msg("error getting gateway contract for processing inbound tracker") | ||
| } | ||
| if err == nil && tx != nil && ethcommon.HexToAddress(tx.To) == gatewayAddr { |
There was a problem hiding this comment.
Only transactions directly calling the gateway contract (i.e., ethcommon.HexToAddress(tx.To) == gatewayAddr) can be successfully processed. But what about transactions where the gateway contract was called by another contract (i.e., tx.To != gatewayAddr)? Is it intentional to exclude such transactions?
There was a problem hiding this comment.
No good point, we will need this as well
Description
This PR fix the
processInboundTrackermethod to allow observer v2 cctx. (deposit, depositAndCall, call)Testing
The current state of the ZetaClient is not yet adapted for proper unit test for this part of the code.
The PR add a E2E test to test inbound tracker processing.
Currently we can't tell the ZetaClient to stop observing inbounds (which would prevent testing the trackers)
The following line must be uncommented to prevent observing inbound:
node/zetaclient/chains/evm/observer/inbound.go
Line 211 in c7d8ec6
Steps:
node/cmd/zetae2e/config/local.yml
Line 100 in 453cb41
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Documentation