feat(zetaclient)!: EVM observer-signer #3390
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 📝 WalkthroughWalkthroughThis pull request introduces a comprehensive refactoring of the orchestrator, specifically focusing on implementing orchestrator V2 for EVM chains. The changes span multiple packages and files, primarily restructuring how EVM observers, signers, and clients are managed. The implementation introduces a more modular and flexible approach to handling cross-chain transactions, with a particular emphasis on improving the EVM observer-signer functionality. Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
Sequence DiagramsequenceDiagram
participant V2 as Orchestrator V2
participant Observer as EVM Observer
participant Signer as EVM Signer
participant Client as EVM Client
V2->>Observer: Bootstrap EVM Chain
Observer->>Client: Initialize RPC Connection
Client-->>Observer: Provide RPC Client
V2->>Signer: Create Signer with Client
Signer->>Client: Use for Transaction Signing
V2->>Observer: Start Observing Transactions
The sequence diagram illustrates the new orchestrator V2 initialization process for EVM chains, highlighting the modular approach to creating observers, signers, and clients. 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
|
|
!!!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 |
|
Please note that #3386 will be addressed in a separate PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (2)
zetaclient/chains/evm/common/validation_test.go (1)
Line range hint
294-416: Consider consolidating duplicate test cases.The individual test cases duplicate the scenarios covered in the table-driven tests. Consider:
- Removing duplicate test cases
- Or if both styles are needed, extract test data and assertions into shared helper functions
// Example helper function: func assertTxValidation(t *testing.T, tx *ethrpc.Transaction, expectedErr string) { err := ValidateEvmTransaction(tx) if expectedErr != "" { require.ErrorContains(t, err, expectedErr) return } require.NoError(t, err) }zetaclient/chains/evm/observer/v2_inbound.go (1)
Line range hint
133-143: Refactor duplicate event validation pattern.The event validation pattern is duplicated across multiple functions. Consider creating a generic event validation function to reduce code duplication.
+func (ob *Observer) validateEvent[T any]( + event *T, + getRaw func(*T) *ethtypes.Log, + gatewayAddr ethcommon.Address, + topics []string, +) error { + raw := getRaw(event) + if err := common.ValidateEvmTxLog(raw, gatewayAddr, "", topics); err != nil { + ob.Logger().Inbound.Warn(). + Err(err). + Msgf("ObserveGateway: invalid event in tx %s on chain %d at height %d", + raw.TxHash.Hex(), ob.Chain().ChainId, raw.BlockNumber) + return err + } + return nil +}Also applies to: 281-291, 412-422
🧹 Nitpick comments (36)
zetaclient/chains/evm/observer/outbound_test.go (1)
426-429: Consider consolidating mock expectations setup.The mock expectations setup could be improved for better maintainability and readability.
Consider consolidating the mock setup into a helper function:
- ob.evmMock.On("BlockNumber", mock.Anything).Unset() - ob.evmMock.On("BlockNumber", mock.Anything).Return(blockNumber+confirmations, nil) - ob.evmMock.On("TransactionByHash", mock.Anything, outboundHash).Return(tx, false, nil) - ob.evmMock.On("TransactionReceipt", mock.Anything, outboundHash).Return(receipt, nil) + setupMockExpectations(ob.evmMock, blockNumber, confirmations, outboundHash, tx, receipt) // Add this helper function: func setupMockExpectations(mock *mocks.EVMClient, blockNumber, confirmations uint64, outboundHash ethcommon.Hash, tx *types.Transaction, receipt *types.Receipt) { mock.On("BlockNumber", mock.Anything).Unset() mock.On("BlockNumber", mock.Anything).Return(blockNumber+confirmations, nil) mock.On("TransactionByHash", mock.Anything, outboundHash).Return(tx, false, nil) mock.On("TransactionReceipt", mock.Anything, outboundHash).Return(receipt, nil) }zetaclient/chains/evm/common/cctx_test.go (1)
Line range hint
11-95: Consider adding edge cases to strengthen test coverage.The test table is well-structured with clear test cases. Consider adding these scenarios:
- Invalid coin type
- Missing inbound params
- Missing CCTX status
{ name: "Invalid coin type", cctx: types.CrossChainTx{ InboundParams: &types.InboundParams{ CoinType: 999, // Invalid coin type }, CctxStatus: &types.Status{ Status: types.CctxStatus_PendingOutbound, }, }, expected: OutboundTypeUnknown, },zetaclient/chains/evm/common/validation_test.go (3)
Line range hint
1-17: Consider using a more robust test data path resolution.While the relative path works, consider using
go-embedor a test helper function to resolve test data paths more reliably://go:embed testdata var testData embed.FS func getTestDataPath() string { // This will work regardless of where the test is executed from return "testdata" }
Line range hint
19-106: Add documentation for test data source.The test cases are comprehensive, but consider adding a comment block explaining:
- The source of the test transaction data
- Why these specific test values were chosen
- The significance of the connector address and topics
Line range hint
108-292: Consider extracting test data setup into helper functions.The test table is comprehensive but could be more maintainable. Consider:
- Creating helper functions for common test data modifications
- Using constants for frequently used test values
- Adding test case grouping comments
func modifyTx(t *testing.T, modify func(*ethrpc.Transaction)) *ethrpc.Transaction { tx := testutils.LoadEVMInbound(t, TestDataDir, chainID, inboundHash, coin.CoinType_Gas) modify(tx) return tx } // Then in tests: { name: "should fail for negative nonce", tx: modifyTx(t, func(tx *ethrpc.Transaction) { tx.Nonce = -1 }), fail: true, msg: "nonce -1 is negative", },zetaclient/chains/evm/client/client.go (2)
61-96: Enhance Error Handling inIsTxConfirmedMethodThe
IsTxConfirmedmethod includes error wrapping but could provide more context for easier debugging.Consider including additional information such as the chain ID or endpoint in error messages:
return false, errors.Wrapf(err, "error getting transaction for tx %s on chain %d", txHash, c.ChainID())Adding relevant details can significantly aid in troubleshooting issues across different chains.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 61-70: zetaclient/chains/evm/client/client.go#L61-L70
Added lines #L61 - L70 were not covered by tests
[warning] 74-80: zetaclient/chains/evm/client/client.go#L74-L80
Added lines #L74 - L80 were not covered by tests
[warning] 84-90: zetaclient/chains/evm/client/client.go#L84-L90
Added lines #L84 - L90 were not covered by tests
[warning] 93-95: zetaclient/chains/evm/client/client.go#L93-L95
Added lines #L93 - L95 were not covered by tests
99-122: Implement Context Timeouts inHealthCheckMethodThe
HealthCheckmethod makes multiple RPC calls without context timeouts, which can lead to blocking operations if the RPC endpoint hangs.Introduce context timeouts to prevent indefinite blocking:
timeoutCtx, cancel := context.WithTimeout(ctx, 5*time.Second) defer cancel() // Use timeoutCtx for all RPC calls bn, err := c.BlockNumber(timeoutCtx)Applying timeouts ensures that the method fails gracefully under network issues or unresponsive endpoints.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 99-104: zetaclient/chains/evm/client/client.go#L99-L104
Added lines #L99 - L104 were not covered by tests
[warning] 107-109: zetaclient/chains/evm/client/client.go#L107-L109
Added lines #L107 - L109 were not covered by tests
[warning] 112-115: zetaclient/chains/evm/client/client.go#L112-L115
Added lines #L112 - L115 were not covered by tests
[warning] 119-121: zetaclient/chains/evm/client/client.go#L119-L121
Added lines #L119 - L121 were not covered by testszetaclient/orchestrator/v2_bootstrap.go (3)
81-85: Clarify Error Message for Chain Type VerificationThe error message when a non-EVM chain is provided could be more informative.
Enhance the error message to include the chain ID or name:
- return nil, errors.New("chain is not EVM") + return nil, errors.Errorf("chain %s (%d) is not an EVM chain", chain.Name(), chain.ID())Providing specific details improves the readability and debuggability of the code.
102-106: Consistency in Error MessagesThe error message in line 104 lacks consistency with other error messages in the method.
For uniformity, adjust the error message to match the style used elsewhere:
- return nil, errors.Wrapf(err, "unable to create http client (%s)", cfg.Endpoint) + return nil, errors.Wrapf(err, "unable to create HTTP client for endpoint %q", cfg.Endpoint)Using
"%q"ensures the endpoint is quoted, matching the style in line 42.
157-169: Placement ofbtcDatabaseFileNameFunctionThe
btcDatabaseFileNamefunction appears in an EVM bootstrapping context, which may cause confusion.Consider relocating
btcDatabaseFileNameto a Bitcoin-specific file to maintain clear separation of chain-specific logic. This enhances code organization and maintainability.zetaclient/chains/evm/evm.go (2)
58-117: Increase test coverage for theStartmethodSeveral critical code paths within the
Startmethod are not covered by unit tests, particularly the error handling scenarios when starting the observer and setting up subscriptions. To enhance reliability and prevent regressions, consider adding unit tests that cover these paths.Do you need assistance in creating test cases for the
Startmethod to improve coverage?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-61: zetaclient/chains/evm/evm.go#L60-L61
Added lines #L60 - L61 were not covered by tests
[warning] 65-66: zetaclient/chains/evm/evm.go#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 70-71: zetaclient/chains/evm/evm.go#L70-L71
Added lines #L70 - L71 were not covered by tests
159-159: Avoid potential precision loss in type conversionThe calculation of
outboundScheduleLookBackinvolves converting afloat64touint64, which may lead to loss of precision or unintended truncation. To prevent this, consider performing the calculation using integer arithmetic.Modify the calculation as follows:
- outboundScheduleLookBack = uint64(float64(lookahead) * outboundLookBackFactor) + outboundScheduleLookBack = lookahead * uint64(outboundLookBackFactor)Ensure that
outboundLookBackFactoris defined as auint64if appropriate.zetaclient/chains/evm/signer/signer.go (2)
238-238: Simplify client usage inbroadcastmethodAccessing
signer.client.EVMRPCClient.SendTransactionsuggests nested dependencies. To improve code clarity and maintainability, consider exposing aSendTransactionmethod directly on theclientor adjusting theclientstructure to avoid deep property chaining.Apply this diff to simplify the method call:
- return signer.client.EVMRPCClient.SendTransaction(ctx, tx) + return signer.client.SendTransaction(ctx, tx)Ensure that
clienthas aSendTransactionmethod that internally handles the transaction sending.
319-319: Enhance logging with consistent message formattingThe log message in
TryProcessOutboundcan be made more informative and consistent with other logs. Including thecctx.Indexprovides better traceability during debugging.Apply this diff to improve the log message:
- logger.Info().Uint64("outbound.nonce", cctx.GetCurrentOutboundParam().TssNonce).Msg("key-sign success") + logger.Info(). + Str("cctx.index", cctx.Index). + Uint64("outbound.nonce", cctx.GetCurrentOutboundParam().TssNonce). + Msg("Key-sign successful for outbound transaction")zetaclient/chains/evm/observer/outbound.go (2)
31-31: Rename function for clarity of purposeThe function
ObserverOutboundreplacesWatchOutbound. To better reflect its functionality of processing outbound trackers, consider renaming it toProcessOutboundTrackers.Apply this diff to rename the function:
-func (ob *Observer) ObserverOutbound(ctx context.Context) error { +func (ob *Observer) ProcessOutboundTrackers(ctx context.Context) error {Update all references to this function accordingly.
355-355: Provide detailed error messages for event parsingWhen
ParseAndCheckWithdrawnEventdoes not find theWithdrawnevent, the returned error lacks context. Enhancing the error message aids in troubleshooting.Apply this diff to improve the error message:
- return nil, errors.New("no ERC20 Withdrawn event found") + return nil, fmt.Errorf("no ERC20 Withdrawn event found in transaction %s", receipt.TxHash.Hex())zetaclient/orchestrator/orchestrator.go (2)
361-361: Document the exclusion of EVM and Bitcoin chains in schedulingThe condition
if chain.IsBitcoin() || chain.IsEVM()skips Bitcoin and EVM chains from scheduling. To enhance code readability and maintainability, add comments explaining that these chains are now managed by orchestrator V2.Apply this diff to include explanatory comments:
if chain.IsBitcoin() || chain.IsEVM() { + // EVM and Bitcoin chains are managed by orchestrator V2 continue }
402-403: Remove obsolete TODO commentsLines 402 and 403 contain outdated comments. Cleaning up obsolete or irrelevant comments helps maintain code clarity.
Apply this diff to remove the outdated comment:
- // Managed by orchestrator V2zetaclient/chains/evm/client/client_live_test.go (1)
13-18: Consider making RPC endpoints configurable via environment variables.Hardcoded RPC endpoints could lead to test failures if services are down or rate-limited. Consider making these configurable via environment variables with fallback values.
const ( - URLEthMainnet = "https://rpc.ankr.com/eth" - URLEthSepolia = "https://rpc.ankr.com/eth_sepolia" - URLBscMainnet = "https://rpc.ankr.com/bsc" - URLPolygonMainnet = "https://rpc.ankr.com/polygon" + defaultURLEthMainnet = "https://rpc.ankr.com/eth" + defaultURLEthSepolia = "https://rpc.ankr.com/eth_sepolia" + defaultURLBscMainnet = "https://rpc.ankr.com/bsc" + defaultURLPolygonMainnet = "https://rpc.ankr.com/polygon" )Add helper function:
func getEndpoint(envVar, defaultURL string) string { if url := os.Getenv(envVar); url != "" { return url } return defaultURL }zetaclient/chains/evm/observer/observer_gas_test.go (1)
Line range hint
16-19: Define gas-related constants for better maintainability.Consider extracting magic numbers into named constants for better readability and maintainability.
const ( gwei = 10e9 + baseGasPrice = 3 + tipGasPrice = 2 anything = mock.Anything )zetaclient/chains/evm/signer/outbound_tracker_reporter.go (1)
Line range hint
45-47: Extract sleep duration into a configurable constant.The 10-second sleep duration is hardcoded. Consider making it configurable for different network conditions.
+const defaultPollingInterval = 10 * time.Second // ... -time.Sleep(10 * time.Second) +time.Sleep(defaultPollingInterval)zetaclient/zetacore/client_subscriptions.go (1)
32-36: Consider implementing exponential backoff for block subscription timeouts.The current implementation uses a fixed 10-second timeout. Consider implementing exponential backoff to handle temporary network issues more gracefully.
type blockTimeout struct { duration time.Duration maxDuration time.Duration multiplier float64 } func (bt *blockTimeout) next() time.Duration { bt.duration = time.Duration(float64(bt.duration) * bt.multiplier) if bt.duration > bt.maxDuration { bt.duration = bt.maxDuration } return bt.duration }pkg/fanout/fanout_test.go (2)
23-25: Consider asserting the ignored errors.While ignoring errors in test context is acceptable, asserting that they are nil would provide additional validation.
-out1, _ := f.Add() -out2, _ := f.Add() -out3, _ := f.Add() +out1, err1 := f.Add() +require.NoError(t, err1) +out2, err2 := f.Add() +require.NoError(t, err2) +out3, err3 := f.Add() +require.NoError(t, err3)
140-203: Consider enhancing memory tracking precision.While the benchmark effectively measures memory stats, consider using
testing.B.ReportMetric()for more precise memory tracking of specific operations.// Example addition: b.ReportMetric(float64(memStatsAfter.Alloc-memStatsBefore.Alloc), "bytes_allocated/op") b.ReportMetric(float64(memStatsAfter.TotalAlloc-memStatsBefore.TotalAlloc), "total_bytes_allocated/op")zetaclient/orchestrator/v2_bootstrap_test.go (2)
46-46: Consider adding timeout context for HasObserverSigner check.The check for observer signer could potentially block indefinitely.
-return ts.HasObserverSigner(chains.BitcoinMainnet.ChainId) +ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) +defer cancel() +return ts.HasObserverSigner(ctx, chains.BitcoinMainnet.ChainId)
71-80: Consider extracting chain configuration to test constants.The EVM chain configurations could be moved to constants or test helpers for reusability.
const ( defaultEthEndpoint = "http://localhost:8545" defaultMaticEndpoint = "http://localhost:8546" ) func defaultEVMConfig(endpoint string) config.EVMConfig { return config.EVMConfig{ Endpoint: endpoint, } }cmd/zetatool/filterdeposit/evm.go (2)
21-21: Consider using a more descriptive import alias.The alias
common2could be confusing. Consider using a more descriptive alias likeevmcommonto better indicate its purpose.-common2 "github.com/zeta-chain/node/zetaclient/chains/evm/common" +evmcommon "github.com/zeta-chain/node/zetaclient/chains/evm/common"
155-157: Add error logging for failed event checks.Consider logging errors when event checks fail to aid in debugging and monitoring.
err := CheckEvmTxLog(&custodyIter.Event.Raw, erc20CustodyAddress, "", common2.TopicsDeposited) +if err != nil { + fmt.Printf("Failed to validate custody event: %v\n", err) +} if err == nil { err := CheckEvmTxLog(&connectorIter.Event.Raw, connectorAddress, "", common2.TopicsZetaSent) +if err != nil { + fmt.Printf("Failed to validate connector event: %v\n", err) +} if err == nil {Also applies to: 167-169
zetaclient/chains/evm/signer/signer_test.go (2)
80-82: Add documentation for the EvmSigner method.Consider adding a documentation comment to explain the purpose and return value of this method.
+// EvmSigner returns the EVM transaction signer associated with the client. func (ts *testSuite) EvmSigner() ethtypes.Signer { return ts.client.Signer }
94-98: Make the nil signer parameter more explicit.Consider using a named variable for the nil signer parameter to improve code readability.
evmMock := mocks.NewEVMRPCClient(t) evmMock.On("BlockNumber", mock.Anything).Return(uint64(1000), nil) -evmClient := client.New(evmMock, nil) +var signer ethtypes.Signer +evmClient := client.New(evmMock, signer) // explicitly passing nil signer for testingzetaclient/chains/evm/observer/v2_outbound.go (2)
Line range hint
70-77: Refactor duplicate error handling pattern.The error handling pattern is duplicated across multiple validation functions. Consider extracting the common pattern into a helper function to improve maintainability and reduce code duplication.
+func validateAndReturnError(vLog *ethtypes.Log, contractAddr ethcommon.Address, txHash string, topics []string) (*big.Int, chains.ReceiveStatus, error) { + if err := common.ValidateEvmTxLog(vLog, contractAddr, txHash, topics); err != nil { + return big.NewInt(0), chains.ReceiveStatus_failed, errors.Wrap(err, "failed to validate event") + } + return nil, chains.ReceiveStatus_success, nil +}Also applies to: 124-131, 184-191, 244-251
Line range hint
124-131: Fix typo in error message.The error message contains a typo: "reverte" should be "reverted".
- "failed to validate gateway reverte event", + "failed to validate gateway reverted event",zetaclient/chains/evm/observer/observer.go (2)
296-305: Enhance error handling in CheckRPCStatus.The function could provide more context about the RPC endpoint in the error message to aid in debugging multi-chain deployments.
func (ob *Observer) CheckRPCStatus(ctx context.Context) error { blockTime, err := ob.evmClient.HealthCheck(ctx) if err != nil { - return errors.Wrap(err, "unable to check rpc health") + return errors.Wrapf(err, "unable to check rpc health for chain %d", ob.Chain().ChainId) } ob.ReportBlockLatency(blockTime) return nil }
38-38: Consider adding documentation for the client type change.The change from
interfaces.EVMRPCClientto*client.Clientis significant. Consider adding a comment explaining the benefits of using the new client type.+ // evmClient is the EVM client that provides enhanced RPC functionality and health checking capabilities evmClient *client.Clientzetaclient/chains/evm/observer/observer_test.go (1)
128-133: Consider extracting mock setup into a helper function.The mock client setup pattern is repeated in multiple places. Consider extracting it into a helper function to improve test maintainability.
+func newMockEVMClient(t *testing.T, blockNumber uint64, err error) *client.Client { + evmMock := mocks.NewEVMRPCClient(t) + evmMock.On("BlockNumber", mock.Anything).Return(blockNumber, err) + return client.New(evmMock, nil) +}zetaclient/zetacore/client_crosschain.go (1)
16-28: LGTM! Clean implementation with proper error handling.The function correctly handles errors and updates metrics only on success. The conversion of chainID to string and total to float64 is done safely.
Consider adding godoc comments to document the function's purpose, parameters, and return values.
+// ListPendingCCTX retrieves pending cross-chain transactions for a given chain. +// It updates the PendingTxsPerChain metrics counter if the operation succeeds. +// Parameters: +// - ctx: Context for the operation +// - chainID: ID of the chain to query +// Returns: +// - List of pending cross-chain transactions +// - Total count of pending transactions +// - Error if the operation fails func (c *Client) ListPendingCCTX(ctx context.Context, chainID int64) ([]*types.CrossChainTx, uint64, error) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
changelog.md(1 hunks)cmd/zetaclientd/inbound.go(3 hunks)cmd/zetatool/filterdeposit/evm.go(3 hunks)pkg/fanout/fanout.go(4 hunks)pkg/fanout/fanout_test.go(3 hunks)zetaclient/chains/bitcoin/bitcoin.go(1 hunks)zetaclient/chains/evm/client/client.go(1 hunks)zetaclient/chains/evm/client/client_live_test.go(1 hunks)zetaclient/chains/evm/common/cctx.go(1 hunks)zetaclient/chains/evm/common/cctx_test.go(7 hunks)zetaclient/chains/evm/common/constant.go(1 hunks)zetaclient/chains/evm/common/validation.go(1 hunks)zetaclient/chains/evm/common/validation_test.go(5 hunks)zetaclient/chains/evm/evm.go(1 hunks)zetaclient/chains/evm/observer/inbound.go(10 hunks)zetaclient/chains/evm/observer/inbound_test.go(18 hunks)zetaclient/chains/evm/observer/observer.go(6 hunks)zetaclient/chains/evm/observer/observer_gas.go(2 hunks)zetaclient/chains/evm/observer/observer_gas_test.go(2 hunks)zetaclient/chains/evm/observer/observer_test.go(10 hunks)zetaclient/chains/evm/observer/outbound.go(4 hunks)zetaclient/chains/evm/observer/outbound_test.go(1 hunks)zetaclient/chains/evm/observer/rpc_status.go(0 hunks)zetaclient/chains/evm/observer/v2_inbound.go(4 hunks)zetaclient/chains/evm/observer/v2_outbound.go(6 hunks)zetaclient/chains/evm/rpc/rpc.go(0 hunks)zetaclient/chains/evm/rpc/rpc_live_test.go(0 hunks)zetaclient/chains/evm/signer/outbound_tracker_reporter.go(2 hunks)zetaclient/chains/evm/signer/sign.go(4 hunks)zetaclient/chains/evm/signer/sign_test.go(1 hunks)zetaclient/chains/evm/signer/signer.go(7 hunks)zetaclient/chains/evm/signer/signer_test.go(6 hunks)zetaclient/chains/evm/signer/v2_signer.go(2 hunks)zetaclient/orchestrator/bootstrap.go(2 hunks)zetaclient/orchestrator/bootstrap_test.go(0 hunks)zetaclient/orchestrator/orchestrator.go(2 hunks)zetaclient/orchestrator/v2_bootstrap.go(2 hunks)zetaclient/orchestrator/v2_bootstrap_test.go(5 hunks)zetaclient/orchestrator/v2_orchestrator.go(2 hunks)zetaclient/orchestrator/v2_orchestrator_test.go(2 hunks)zetaclient/testutils/testrpc/rpc_evm.go(1 hunks)zetaclient/zetacore/client_crosschain.go(1 hunks)zetaclient/zetacore/client_subscriptions.go(3 hunks)
💤 Files with no reviewable changes (4)
- zetaclient/orchestrator/bootstrap_test.go
- zetaclient/chains/evm/rpc/rpc_live_test.go
- zetaclient/chains/evm/observer/rpc_status.go
- zetaclient/chains/evm/rpc/rpc.go
✅ Files skipped from review due to trivial changes (4)
- zetaclient/chains/evm/common/cctx.go
- zetaclient/chains/evm/common/constant.go
- zetaclient/chains/evm/observer/inbound_test.go
- zetaclient/chains/evm/common/validation.go
🧰 Additional context used
📓 Path-based instructions (34)
zetaclient/testutils/testrpc/rpc_evm.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/orchestrator/bootstrap.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/signer/sign_test.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_inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/orchestrator/v2_orchestrator.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetatool/filterdeposit/evm.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/zetacore/client_crosschain.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/signer/outbound_tracker_reporter.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/orchestrator/v2_orchestrator_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/common/validation_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/orchestrator/v2_bootstrap_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/outbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/signer/v2_signer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/outbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/observer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/client/client.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.
zetaclient/orchestrator/v2_bootstrap.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/bitcoin.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetaclientd/inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/client/client_live_test.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_outbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/signer/signer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/observer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
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/evm/signer/sign.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/fanout/fanout_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/fanout/fanout.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/evm.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/common/cctx_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/observer_gas.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/observer_gas_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/zetacore/client_subscriptions.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 GitHub Check: codecov/patch
zetaclient/chains/evm/client/client.go
[warning] 26-33: zetaclient/chains/evm/client/client.go#L26-L33
Added lines #L26 - L33 were not covered by tests
[warning] 35-38: zetaclient/chains/evm/client/client.go#L35-L38
Added lines #L35 - L38 were not covered by tests
[warning] 40-43: zetaclient/chains/evm/client/client.go#L40-L43
Added lines #L40 - L43 were not covered by tests
[warning] 45-50: zetaclient/chains/evm/client/client.go#L45-L50
Added lines #L45 - L50 were not covered by tests
[warning] 52-54: zetaclient/chains/evm/client/client.go#L52-L54
Added lines #L52 - L54 were not covered by tests
[warning] 57-58: zetaclient/chains/evm/client/client.go#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 61-70: zetaclient/chains/evm/client/client.go#L61-L70
Added lines #L61 - L70 were not covered by tests
[warning] 74-80: zetaclient/chains/evm/client/client.go#L74-L80
Added lines #L74 - L80 were not covered by tests
[warning] 84-90: zetaclient/chains/evm/client/client.go#L84-L90
Added lines #L84 - L90 were not covered by tests
[warning] 93-95: zetaclient/chains/evm/client/client.go#L93-L95
Added lines #L93 - L95 were not covered by tests
[warning] 99-104: zetaclient/chains/evm/client/client.go#L99-L104
Added lines #L99 - L104 were not covered by tests
[warning] 107-109: zetaclient/chains/evm/client/client.go#L107-L109
Added lines #L107 - L109 were not covered by tests
[warning] 112-115: zetaclient/chains/evm/client/client.go#L112-L115
Added lines #L112 - L115 were not covered by tests
[warning] 119-121: zetaclient/chains/evm/client/client.go#L119-L121
Added lines #L119 - L121 were not covered by tests
zetaclient/chains/bitcoin/bitcoin.go
[warning] 130-132: zetaclient/chains/bitcoin/bitcoin.go#L130-L132
Added lines #L130 - L132 were not covered by tests
pkg/fanout/fanout.go
[warning] 128-129: pkg/fanout/fanout.go#L128-L129
Added lines #L128 - L129 were not covered by tests
[warning] 140-140: pkg/fanout/fanout.go#L140
Added line #L140 was not covered by tests
zetaclient/chains/evm/evm.go
[warning] 60-61: zetaclient/chains/evm/evm.go#L60-L61
Added lines #L60 - L61 were not covered by tests
[warning] 65-66: zetaclient/chains/evm/evm.go#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 70-71: zetaclient/chains/evm/evm.go#L70-L71
Added lines #L70 - L71 were not covered by tests
[warning] 131-134: zetaclient/chains/evm/evm.go#L131-L134
Added lines #L131 - L134 were not covered by tests
[warning] 136-139: zetaclient/chains/evm/evm.go#L136-L139
Added lines #L136 - L139 were not covered by tests
[warning] 141-165: zetaclient/chains/evm/evm.go#L141-L165
Added lines #L141 - L165 were not covered by tests
[warning] 167-170: zetaclient/chains/evm/evm.go#L167-L170
Added lines #L167 - L170 were not covered by tests
[warning] 172-189: zetaclient/chains/evm/evm.go#L172-L189
Added lines #L172 - L189 were not covered by tests
[warning] 193-203: zetaclient/chains/evm/evm.go#L193-L203
Added lines #L193 - L203 were not covered by tests
[warning] 209-214: zetaclient/chains/evm/evm.go#L209-L214
Added lines #L209 - L214 were not covered by tests
[warning] 216-218: zetaclient/chains/evm/evm.go#L216-L218
Added lines #L216 - L218 were not covered by tests
[warning] 222-224: zetaclient/chains/evm/evm.go#L222-L224
Added lines #L222 - L224 were not covered by tests
[warning] 227-238: zetaclient/chains/evm/evm.go#L227-L238
Added lines #L227 - L238 were not covered by tests
🔇 Additional comments (28)
zetaclient/chains/evm/observer/outbound_test.go (1)
426-429: Verify error handling test coverage.The test suite includes a happy path test but could benefit from additional error scenarios.
Consider adding test cases for:
- Failed block number retrieval
- Failed transaction retrieval
- Failed receipt retrieval
Would you like me to generate additional test cases to cover these error scenarios?
zetaclient/chains/evm/common/cctx_test.go (2)
1-9: LGTM! Well-structured package and imports.The package name change to
commonaligns with the module restructuring, and imports follow Go best practices.
Line range hint
97-103: LGTM! Clean test execution logic.The test execution follows Go testing best practices with proper error handling.
pkg/fanout/fanout.go (1)
128-129: Test Coverage for Channel Closure LogicThe code segments responsible for channel closure (lines 128-129, 135-141) are not covered by tests.
To ensure the reliability of the
closemethod, please add unit tests that verify the channel closes correctly after pending writes complete.Also applies to: 135-141
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 128-129: pkg/fanout/fanout.go#L128-L129
Added lines #L128 - L129 were not covered by testszetaclient/orchestrator/bootstrap.go (1)
102-103: Ensure consistency in handling orchestrator V2 chainsThe comments and
continuestatements indicate that EVM and Bitcoin chains are now managed by orchestrator V2. Ensure that this logic consistently applies across all relevant functions to prevent unintended behavior.Also applies to: 256-257
zetaclient/chains/evm/signer/signer.go (1)
175-175: Verify transaction hash computation methodIn the
Signmethod, the transaction hash is computed usingsigner.client.Hash(tx). Confirm that theHashmethod correctly computes the hash for all supported transaction types, including legacy and EIP-1559 transactions, to prevent potential issues with transaction signing.zetaclient/chains/evm/observer/outbound.go (2)
23-23: Validate new import path for dependenciesThe import path has been updated to
"github.com/zeta-chain/node/zetaclient/chains/evm/common". Ensure that this path correctly references the necessary packages and that all dependencies are properly managed to avoid build issues.
Line range hint
298-319: Ensure robust event validation and error handlingIn
ParseAndCheckZetaEvent, when parsing events, make sure that the validation withcommon.ValidateEvmTxLogis thorough. Confirm that all expected scenarios are handled, and consider logging additional context in case of errors to facilitate debugging.zetaclient/chains/evm/observer/inbound.go (1)
10-10: Ensure Go 1.21 compatibility due to usage ofslicespackageThe
slicespackage was introduced in Go 1.21. Please verify that the project is configured to use Go 1.21 or later. If backward compatibility with earlier Go versions is required, consider implementing an alternative method to calculate the minimum value.zetaclient/testutils/testrpc/rpc_evm.go (1)
27-31: Addition ofSetChainIDmethod is correctThe
SetChainIDmethod appropriately mocks theeth_chainIdRPC response, enhancing the flexibility of the test utilities.zetaclient/chains/evm/signer/v2_signer.go (1)
10-10: Refactor to usecommonpackage constants enhances modularityUpdating the import to the
commonpackage and referencing outbound type constants from this package improves code modularity and consistency across the project.Also applies to: 19-33
zetaclient/zetacore/client_subscriptions.go (1)
37-39: LGTM: Proper cleanup of resources.Good implementation of resource cleanup using the closeConsumer function when the context is done.
pkg/fanout/fanout_test.go (1)
77-137: Well-structured test with clear sections and proper concurrency handling.The test effectively validates the fanout behavior with channel closure, using atomic operations for thread-safe counting.
zetaclient/chains/evm/signer/sign.go (3)
13-13: LGTM! Import path update aligns with package reorganization.The change from
evmtocommonpackage improves code organization.
111-111: LGTM! Consistent use of relocated constant.The gas limit constant is now correctly sourced from the common package.
130-130: LGTM! Consistent use of relocated constant.The gas limit constant is now correctly sourced from the common package.
zetaclient/orchestrator/v2_bootstrap_test.go (2)
23-24: LGTM! Proper test parallelization.Good practice to mark independent tests as parallel for improved test execution time.
123-160: Well-structured table-driven tests with comprehensive coverage.Good use of table-driven tests to validate database file names across different Bitcoin chains.
zetaclient/chains/evm/signer/sign_test.go (1)
51-51: LGTM! Consistent with signer implementation refactoring.The change correctly uses the new client.Signer field instead of the deprecated EvmSigner() method.
zetaclient/orchestrator/v2_orchestrator.go (2)
156-156: LGTM! EVM chain bootstrapping implementation.The implementation follows the established pattern for chain bootstrapping and is correctly placed within the chain type switch.
Line range hint
168-174: LGTM! Improved error handling.The use of
errors.Isfor error comparison is more idiomatic Go and provides better error handling semantics.zetaclient/orchestrator/v2_orchestrator_test.go (2)
168-174: LGTM! Thread-safe implementation.The
HasObserverSignermethod correctly implements thread-safe access to the shared map using a mutex.
200-202: LGTM! Added thread safety to configuration updates.The mutex protection is correctly implemented to ensure thread-safe configuration updates.
zetaclient/chains/evm/signer/signer_test.go (1)
42-44: LGTM! Good separation of mock and actual client.The separation of
evmMockandclientfields provides better testing flexibility and clearer intent.zetaclient/chains/evm/observer/observer_test.go (1)
389-390: Well-structured separation of mock and client fields.Good practice separating the mock client from the wrapped client instance. This makes the test setup clearer and more maintainable.
zetaclient/chains/bitcoin/bitcoin.go (2)
130-132: 🛠️ Refactor suggestionAdd test coverage for chain params update.
This critical error handling code in the scheduler is not covered by tests. While the implementation is correct, we should ensure it's tested to maintain reliability.
Run this script to verify test coverage:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 130-132: zetaclient/chains/bitcoin/bitcoin.go#L130-L132
Added lines #L130 - L132 were not covered by tests
Line range hint
133-196: Consider adding mutex protection for concurrent processor access.The function launches goroutines to process outbound transactions while sharing the processor state. While the processor likely has internal synchronization, it would be safer to document or verify this assumption.
The overall implementation is well-structured with proper error handling and logging. The use of goroutines for non-blocking processing is a good pattern.
Run this script to verify concurrent access safety:
✅ Verification successful
Concurrent access is properly protected
The Processor implementation in outboundprocessor package uses appropriate mutex synchronization for all shared state access. All methods (StartTryProcess, EndTryProcess, IsOutboundActive) properly acquire and release locks, making the concurrent goroutine usage in bitcoin.go safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for concurrent access patterns in processor rg -A 5 "type Processor struct" ./zetaclient/outboundprocessor/Length of output: 595
Script:
#!/bin/bash # Check mutex usage in processor methods ast-grep --pattern 'func (p *Processor) $_ ($$$) { $$$ }' ./zetaclient/outboundprocessor/outbound_processor.goLength of output: 1700
Script:
#!/bin/bash # Check IsOutboundActive implementation ast-grep --pattern 'func (p *Processor) IsOutboundActive($$$) { $$$ }' ./zetaclient/outboundprocessor/outbound_processor.goLength of output: 127
Script:
#!/bin/bash # Search for IsOutboundActive with a more flexible pattern rg -A 5 "IsOutboundActive" ./zetaclient/outboundprocessor/outbound_processor.goLength of output: 299
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 130-132: zetaclient/chains/bitcoin/bitcoin.go#L130-L132
Added lines #L130 - L132 were not covered by testschangelog.md (1)
11-11: LGTM! Clear and well-formatted changelog entry.The entry follows the changelog format and is properly categorized under the "Refactor" section.
… into feat/evm-orchestrator-v2
# Conflicts: # cmd/zetaclientd/inbound.go # zetaclient/orchestrator/v2_bootstrap.go
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This PR implements V2 orchestrator for EVM-based chains similar to BTC in #3332. Note that observing/scheduling logic left as is.
EVMobserverSignerfanoutpackageCloses #3302
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the release notes:
Refactor
New Features
Chores