-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: update ethermint for event fix (release v20) #2858
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 WalkthroughWalkthroughThe pull request introduces significant updates across multiple files, primarily focusing on enhancing the testing framework, optimizing workflow configurations, and implementing new functionalities related to Bitcoin and ERC20 custody contract migrations. Key changes include the consolidation of input parameters in GitHub Actions, the addition of new test cases for Bitcoin inscription memos, and the introduction of a migration process for upgrading custody contracts. The modifications also encompass updates to various scripts, configuration files, and the Docker setup, reflecting a comprehensive enhancement of the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHubActions as GA
participant Makefile
participant E2ETests as E2E
participant BitcoinClient as BTC
participant ERC20Custody as ERC20
participant ZetaChain as ZC
User->>GA: Trigger Workflow
GA->>Makefile: Execute Tests
Makefile->>E2E: Start Tests
E2E->>BTC: Execute Bitcoin Inscription Test
BTC-->>E2E: Return Results
E2E->>ERC20: Execute Migration Tests
ERC20-->>E2E: Return Migration Results
E2E-->>GA: Return Overall Test Results
GA-->>User: Notify Completion
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
Outside diff range, codebase verification and nitpick comments (5)
e2e/txserver/authority.go (1)
11-33
: Improve error handling and add parameter validation.Consider the following suggestions to improve the code:
Simplify the error handling by using
return fmt.Errorf()
directly instead of returningerr
and then wrapping it withfmt.Errorf()
.Validate the
msgURL
parameter before broadcasting the transaction to ensure it is a valid URL.Apply this diff to implement the suggestions:
func (zts ZetaTxServer) AddAuthorization(msgURL string) error { // retrieve account accAdmin, err := zts.clientCtx.Keyring.Key(e2eutils.AdminPolicyName) if err != nil { - return err + return fmt.Errorf("failed to retrieve admin account: %w", err) } addrAdmin, err := accAdmin.GetAddress() if err != nil { - return err + return fmt.Errorf("failed to get admin address: %w", err) } + // validate msgURL + if _, err := url.ParseRequestURI(msgURL); err != nil { + return fmt.Errorf("invalid msgURL: %w", err) + } + // add new authorization _, err = zts.BroadcastTx(e2eutils.AdminPolicyName, authoritytypes.NewMsgAddAuthorization( addrAdmin.String(), msgURL, authoritytypes.PolicyType_groupAdmin, )) if err != nil { - return fmt.Errorf("failed to add authorization: %w", err) + return fmt.Errorf("failed to broadcast transaction: %w", err) } return nil }x/fungible/keeper/msg_server_update_gateway_contract.go (1)
Line range hint
66-84
: Move the event emission to a separate function.The event emission can be moved to a separate function to improve readability and reusability:
func (k msgServer) emitGatewayContractUpdatedEvent( ctx sdk.Context, msg *types.MsgUpdateGatewayContract, oldGateway string, ) error { err := ctx.EventManager().EmitTypedEvent( &types.EventGatewayContractUpdated{ MsgTypeUrl: sdk.MsgTypeURL(&types.MsgUpdateGatewayContract{}), NewContractAddress: msg.NewGatewayContractAddress, OldContractAddress: oldGateway, Signer: msg.Creator, }, ) if err != nil { k.Logger(ctx).Error("failed to emit event", "error", err.Error()) return errors.Wrap(err, "failed to emit event") } return nil }Then, call this function at the end of
UpdateGatewayContract
:if err := k.emitGatewayContractUpdatedEvent(ctx, msg, oldGateway); err != nil { return nil, err }e2e/runner/v2_setup_evm.go (1)
25-25
: Approved with a minor suggestion.The changes to the
SetupEVMV2
method enhance its functionality and improve logging practices. The new transaction to set legacy support in the ERC20 custody contract is implemented correctly, with proper error handling and receipt validation.Consider extracting the repeated logging statements into a separate function to improve code reusability and maintainability.
func logSetupDuration(logger *zap.SugaredLogger, message string, startTime time.Time) { logger.Info("%s took %s\n", message, time.Since(startTime)) }Then, replace the logging statements with:
logSetupDuration(r.Logger, "EVM v2 setup", startTime)Also applies to: 107-110, 112-112
e2e/runner/bitcoin.go (1)
299-340
: LGTM, but address the TODO comment.The new function
InscribeToTSSFromDeployerWithMemo
is a valuable addition to theE2ERunner
, enhancing its transaction capabilities by allowing it to handle inscriptions with memos. The code is well-structured and follows a clear flow, making it easy to understand and maintain.However, please ensure that the TODO comment at line 305 is addressed in a timely manner. Replacing the
InscriptionBuilder
with a Go function to enable instructions will improve the code quality and maintainability.e2e/e2etests/e2etests.go (1)
Line range hint
1-1
: TODO: Add tests to ensure code correctness and prevent regressions.The TODO comment indicates that tests are missing for this file. As a best practice, it is crucial to have comprehensive test coverage.
If you need any guidance or assistance in writing the tests, please let me know. I'd be happy to help you get started or review your test cases.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
go.sum
is excluded by!**/*.sum
Files selected for processing (48)
- .github/workflows/e2e.yml (7 hunks)
- Makefile (1 hunks)
- changelog.md (3 hunks)
- cmd/zetae2e/config/localnet.yml (1 hunks)
- cmd/zetae2e/local/local.go (5 hunks)
- cmd/zetae2e/local/v2.go (1 hunks)
- contrib/localnet/bitcoin-sidecar/Dockerfile (1 hunks)
- contrib/localnet/bitcoin-sidecar/js/package.json (1 hunks)
- contrib/localnet/bitcoin-sidecar/js/src/client.ts (1 hunks)
- contrib/localnet/bitcoin-sidecar/js/src/index.ts (1 hunks)
- contrib/localnet/bitcoin-sidecar/js/src/script.ts (1 hunks)
- contrib/localnet/bitcoin-sidecar/js/src/tsconfig.json (1 hunks)
- contrib/localnet/bitcoin-sidecar/js/src/util.ts (1 hunks)
- contrib/localnet/docker-compose.yml (1 hunks)
- contrib/localnet/orchestrator/start-zetae2e.sh (1 hunks)
- contrib/localnet/scripts/start-zetacored.sh (1 hunks)
- e2e/config/config.go (3 hunks)
- e2e/e2etests/e2etests.go (2 hunks)
- e2e/e2etests/test_extract_bitcoin_inscription_memo.go (1 hunks)
- e2e/runner/accounting.go (1 hunks)
- e2e/runner/bitcoin.go (3 hunks)
- e2e/runner/evm.go (2 hunks)
- e2e/runner/inscription.go (1 hunks)
- e2e/runner/solana.go (2 hunks)
- e2e/runner/v2_migration.go (1 hunks)
- e2e/runner/v2_setup_evm.go (2 hunks)
- e2e/runner/v2_setup_zeta.go (2 hunks)
- e2e/txserver/authority.go (1 hunks)
- go.mod (2 hunks)
- pkg/contracts/solana/gateway.go (1 hunks)
- x/fungible/keeper/msg_server_update_gateway_contract.go (3 hunks)
- x/fungible/keeper/msg_server_update_gateway_contract_test.go (3 hunks)
- x/fungible/keeper/v2_evm.go (1 hunks)
- zetaclient/chains/bitcoin/fee.go (2 hunks)
- zetaclient/chains/bitcoin/observer/inbound.go (3 hunks)
- zetaclient/chains/bitcoin/observer/observer.go (2 hunks)
- zetaclient/chains/bitcoin/observer/witness.go (5 hunks)
- zetaclient/chains/bitcoin/rpc/rpc.go (3 hunks)
- zetaclient/chains/bitcoin/rpc/rpc_live_test.go (10 hunks)
- zetaclient/chains/evm/signer/signer.go (1 hunks)
- zetaclient/chains/interfaces/interfaces.go (1 hunks)
- zetaclient/chains/solana/observer/inbound.go (4 hunks)
- zetaclient/chains/solana/observer/inbound_test.go (5 hunks)
- zetaclient/chains/solana/signer/withdraw.go (1 hunks)
- zetaclient/orchestrator/orchestrator.go (4 hunks)
- zetaclient/testdata/solana/chain_901_inbound_tx_result_MS3MPLN7hkbyCZFwKqXcg8fmEvQMD74fN6Ps2LSWXJoRxPW5ehaxBorK9q1JFVbqnAvu9jXm6ertj7kT7HpYw1j.json (1 hunks)
- zetaclient/testutils/constant.go (1 hunks)
- zetaclient/testutils/mocks/solana_rpc.go (2 hunks)
Files skipped from review due to trivial changes (4)
- contrib/localnet/bitcoin-sidecar/js/package.json
- contrib/localnet/bitcoin-sidecar/js/src/tsconfig.json
- contrib/localnet/bitcoin-sidecar/js/src/util.ts
- zetaclient/testutils/constant.go
Additional context used
Path-based instructions (33)
e2e/txserver/authority.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_extract_bitcoin_inscription_memo.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/contracts/solana/gateway.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/fungible/keeper/msg_server_update_gateway_contract.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/inscription.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/v2_setup_zeta.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/v2_setup_evm.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.cmd/zetae2e/local/v2.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/solana/signer/withdraw.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/fungible/keeper/v2_evm.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/solana.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/rpc/rpc.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/fungible/keeper/msg_server_update_gateway_contract_test.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/observer/witness.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/accounting.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/solana/observer/inbound_test.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/v2_migration.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/evm.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/interfaces/interfaces.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.contrib/localnet/orchestrator/start-zetae2e.sh (1)
Pattern
**/*.sh
: Review the shell scripts, point out issues relative to security, performance, and maintainability.zetaclient/testutils/mocks/solana_rpc.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/fee.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/solana/observer/inbound.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/bitcoin.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/config/config.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/observer/inbound.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.cmd/zetae2e/local/local.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.contrib/localnet/scripts/start-zetacored.sh (1)
Pattern
**/*.sh
: Review the shell scripts, point out issues relative to security, performance, and maintainability.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/bitcoin/rpc/rpc_live_test.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/observer/observer.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.e2e/e2etests/e2etests.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Learnings (1)
zetaclient/orchestrator/orchestrator.go (1)
Learnt from: ws4charlie PR: zeta-chain/node#2411 File: zetaclient/orchestrator/orchestrator.go:192-217 Timestamp: 2024-07-04T23:46:38.428Z Learning: The `GetUpdatedSigner` method in `zetaclient/orchestrator/orchestrator.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go` and `zetaclient/orchestrator/orchestrator_test.go`.
Gitleaks
cmd/zetae2e/config/localnet.yml
47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
51-51: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
55-55: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
59-59: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Biome
contrib/localnet/bitcoin-sidecar/js/src/client.ts
[error] 17-17: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
[error] 18-18: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
[error] 21-21: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
Additional comments not posted (74)
contrib/localnet/bitcoin-sidecar/Dockerfile (1)
1-14
: LGTM!The Dockerfile follows best practices by using a multi-stage build process to reduce the final image size. The build process is straightforward and there are no apparent issues.
contrib/localnet/bitcoin-sidecar/js/src/index.ts (1)
1-38
: LGTM!The code changes are approved. The Express server setup and route handling look good.
e2e/e2etests/test_extract_bitcoin_inscription_memo.go (1)
1-60
: LGTM!The code changes are approved. The test follows a clear and logical flow, and the usage of helper functions and packages looks good.
pkg/contracts/solana/gateway.go (1)
16-18
: LGTM, but verify the change is correct.The code change is approved.
However, ensure that changing the
AccountsNumDeposit
constant from 4 to 3 is indeed correct and doesn't break any functionality that relies on this constant.Run the following script to verify the usage of
AccountsNumDeposit
:Verification successful
Verification Successful: Change to
AccountsNumDeposit
is CorrectThe change of
AccountsNumDeposit
from 4 to 3 is consistent with its usage in the codebase. The logic inzetaclient/chains/solana/observer/inbound.go
aligns with the new value, and the comments support this adjustment. No issues were found with this modification.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `AccountsNumDeposit` constant. # Test: Search for the usage of the constant. Expect: Only occurrences with the new value 3. rg --type go -A 5 $'AccountsNumDeposit'Length of output: 1239
zetaclient/testdata/solana/chain_901_inbound_tx_result_MS3MPLN7hkbyCZFwKqXcg8fmEvQMD74fN6Ps2LSWXJoRxPW5ehaxBorK9q1JFVbqnAvu9jXm6ertj7kT7HpYw1j.json (1)
1-64
: This is a test data file and does not require a code review.e2e/runner/inscription.go (1)
30-34
: LGTM!The
InscriptionBuilder
struct is well-defined and serves a clear purpose.e2e/runner/v2_setup_zeta.go (1)
78-80
: LGTM!The changes to the
UpdateChainParamsV2Contracts
method are well-structured and clearly documented. The updated functionality to set both the ERC20 custody contract address and the gateway address in the chain parameters is implemented correctly.The code changes are approved.
Also applies to: 104-106
cmd/zetae2e/local/v2.go (1)
15-72
: LGTM!The
startV2Tests
function is well-designed and follows a clear pattern for initiating version 2 (v2) end-to-end tests. The use oferrgroup.Group
to run test routines concurrently enhances the efficiency of the testing process.The test cases cover a broad range of scenarios, including happy paths and revert cases for both gas token and ERC20 token workflows. This ensures thorough testing of successful transactions and error handling.
The function is appropriately named, and its purpose is clear.
The code changes are approved.
zetaclient/chains/solana/signer/withdraw.go (1)
72-74
: Approved: Method name change is appropriate.The change from
GetRecentBlockhash
toGetLatestBlockhash
is a suitable refactor that improves code clarity without altering functionality. The error message has also been updated accordingly.cmd/zetae2e/config/localnet.yml (2)
44-59
: Approved: Addition of new user accounts for testing or development purposes.The new user account configurations, namely
user_v2_ether
,user_v2_erc20
,user_v2_ether_revert
, anduser_v2_erc20_revert
, have been added to thelocalnet.yml
file. Each account includes the necessary fields:bech32_address
,evm_address
, andprivate_key
. This addition enhances the configuration capabilities for testing or development scenarios related to Ethereum and ERC20 token interactions.Tools
Gitleaks
47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
51-51: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
55-55: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
59-59: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
47-47
: Skipped: False positive from static analysis tool.The flagged
private_key
fields are not API keys but private keys associated with the user accounts. They are used for signing transactions and are meant to be kept private within the configuration file. The static analysis tool's flag can be safely ignored as a false positive in this context.Also applies to: 51-51, 55-55, 59-59
Tools
Gitleaks
47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
x/fungible/keeper/v2_evm.go (1)
18-42
: Approved: Addition ofCallUpdateGatewayAddress
function to interact with ZRC20 contract.The new
CallUpdateGatewayAddress
function has been added to theKeeper
struct to enable updating the gateway address associated with a ZRC20 contract. The function retrieves the ABI for the ZRC20 contract usingzrc20.ZRC20MetaData.GetAbi()
and handles the potential error appropriately. It then proceeds to call theupdateGatewayAddress
function on the ZRC20 contract usingCallEVM
, passing the necessary parameters.This addition enhances the functionality of the
Keeper
struct by providing a way to interact with the ZRC20 contract and update the gateway address. The function follows the existing pattern of usingCallEVM
to execute EVM transactions, maintaining consistency with the codebase.e2e/runner/solana.go (2)
Line range hint
32-38
: LGTM!The removal of the line that appends the program ID to the account slice is approved. This change suggests that the program ID is no longer necessary for deposit instructions, indicating a shift in how it is handled.
67-67
: LGTM!The update of the function call from
GetRecentBlockhash
toGetLatestBlockhash
is approved. This change likely reflects an update in the Solana client library to better represent the functionality or align with new conventions. The overall logic of creating a signed transaction remains intact.zetaclient/chains/bitcoin/rpc/rpc.go (2)
57-69
: LGTM!The addition of the
GetRawTxByHash
function is approved. This function enhances the RPC client's capabilities by allowing it to fetch transaction details directly, which is utilized in the modifiedGetTransactionFeeAndRate
function to calculate both the transaction fee and the fee rate based on the inputs and outputs of the transaction.
126-174
: LGTM!The changes to the
GetTransactionFeeAndRate
function are approved. The function now calculates both the transaction fee and the fee rate based on the inputs and outputs of the transaction, rather than relying on historical fee rates from recent blocks. This reflects a move towards a more direct and potentially more reliable method of calculating transaction fees, eliminating reliance on historical data and improving the accuracy of fee estimations.The updated error handling ensures that the function gracefully handles potential issues when fetching previous transactions and calculating total input and output values, enhancing the robustness of the code.
x/fungible/keeper/msg_server_update_gateway_contract_test.go (4)
18-91
: LGTM!The enhancements to the "can update the gateway contract address stored in the module and update address in ZRC20s" test case are approved. The test now provides a more comprehensive validation of the gateway contract update process by verifying that the new gateway address is correctly reflected in both the system contract and the ZRC20 contracts. This broader scope of testing ensures the correctness of the functionality.
The additional setup for gas coins across two chains is crucial for the new functionality being tested, and the introduction of the
queryZRC20Gateway
helper function improves the readability and maintainability of the test code by encapsulating the logic for retrieving the gateway address from a ZRC20 contract using the Ethereum ABI.These changes enhance the overall quality and robustness of the test suite.
Line range hint
94-129
: Skipping review.The "can update and overwrite the gateway contract if system contract state variable not found" test case remains unchanged. This indicates that the test case is still relevant and valid for the current implementation.
145-145
: LGTM!The update to the message creation using
types.NewMsgUpdateGatewayContract
instead oftypes.NewMsgUpdateSystemContract
is approved. This change aligns with the focus on updating the gateway contract specifically, rather than the system contract as a whole. The rest of the test case logic remains unchanged, ensuring that unauthorized attempts to update the gateway contract are still prevented.
156-178
: LGTM!The addition of the "should prevent update the gateway contract if invalid gateway address" test case is approved. This new test case enhances the robustness of the contract update functionality by ensuring that the system correctly prevents updates when the provided gateway address is not valid.
Covering this important edge case helps maintain the integrity of the gateway contract update process and improves the overall reliability of the system. The test case is well-structured and clearly verifies the expected behavior when an invalid gateway address is provided.
Great work on adding this valuable test case!
contrib/localnet/bitcoin-sidecar/js/src/client.ts (1)
39-111
: LGTM!The
ZetaBtcClient
class and its methods are implemented correctly. The class provides a way to interact with ZetaChain from Bitcoin by generating a P2TR address and a reveal transaction. Thecall
method checks the length ofcalldata
and uses the appropriate method to generate a Bitcoin address. ThecallWithWitness
method generates a leaf script and a script tree to create a P2TR address. ThebuildRevealTxn
method builds a reveal transaction using theRevealTxnBuilder
class. ThegenLeafScript
method generates a leaf script by pushing the public key and data to the script.zetaclient/chains/bitcoin/observer/witness.go (3)
19-20
: LGTM!The changes in the
GetBtcEventWithWitness
function enhance its functionality by introducing additional checks and logic for handling Bitcoin transactions. The new conditional block ensures that the appropriate fee calculation method is used based on the network parameters and the block number. The refined error handling simplifies the error handling flow and avoids unnecessary error returns. The elevated logging level for certain messages improves the visibility of important information.Also applies to: 39-58, 72-78
136-136
: LGTM!The changes in the
tryExtractOpRet
function are minor and only involve updating the comment for clarity. The removal of unnecessary slashes improves the readability of the documentation.
155-155
: LGTM!The changes in the
tryExtractInscription
function are minor and only involve updating the comment for clarity. The removal of unnecessary slashes improves the readability of the documentation.e2e/runner/accounting.go (1)
173-182
: LGTM!The changes in the
checkERC20TSSBalance
function introduce a new logic flow for handling the balance of an ERC20 custody contract. The conditional check ensures that the balance of the new contract is only queried when necessary, optimizing the function's performance by avoiding redundant calls. The computation of the finalcustodyFullBalance
by adding the existingcustodyBalance
to the newly fetchedcustodyV2Balance
when applicable enhances the function's efficiency and correctness.zetaclient/chains/solana/observer/inbound_test.go (4)
29-30
: LGTM!The transaction hash update is approved as it ensures the test references the correct archived transaction data.
54-55
: LGTM!The transaction hash update is approved as it ensures the test references the correct archived transaction data.
64-64
: LGTM!The change to dynamically retrieve the gateway address based on the chain ID is approved. This enhances the flexibility of the tests by ensuring that the gateway address corresponds to the specific chain being tested.
159-160
: LGTM!The changes are approved as they:
- Update the test to reference the correct archived transaction data.
- Align the test with the new expected sender information.
- Reflect a shift in the expected transaction values for the tests.
- Update the expected metadata associated with the transactions.
- Enhance the flexibility of the tests by ensuring that the gateway address corresponds to the specific chain being tested.
Also applies to: 172-172, 177-177, 183-184
e2e/runner/v2_migration.go (1)
1-212
: LGTM!The code for the v2 migration process is well-structured, follows a logical flow, and adheres to best practices. It effectively handles the necessary steps, including adding new admin authorizations, deploying v2 contracts on the EVM chain, upgrading ZRC20s, deploying the gateway on ZetaChain, and migrating ERC20 custody funds.
The code demonstrates appropriate error handling, logging, and usage of testing frameworks and assertions. No major issues were identified during the review.
Great job on implementing this critical migration process!
e2e/runner/evm.go (2)
188-188
: LGTM!The changes to the
ApproveETHZRC20
function are approved. The updated logic allows for more efficient token management by triggering approvals when the allowance falls below a threshold of 1,000, rather than strictly checking for a zero allowance.The introduction of the
thousand
variable improves code readability by clearly defining the threshold value. Additionally, modifying the parameter passed to theAllowance
method aligns the logic with the new approval condition.These enhancements potentially optimize the token approval process and reduce unnecessary transactions in scenarios where allowances may not need to be completely reset.
Also applies to: 191-193
205-205
: LGTM!The changes to the
ApproveERC20ZRC20
function are approved. Similar to theApproveETHZRC20
function, the updated logic allows for more efficient token management by triggering approvals when the allowance falls below a threshold of 1,000, rather than strictly checking for a zero allowance.The introduction of the
thousand
variable improves code readability by clearly defining the threshold value. Additionally, modifying the parameter passed to theAllowance
method aligns the logic with the new approval condition.These enhancements potentially optimize the token approval process and reduce unnecessary transactions in scenarios where allowances may not need to be completely reset.
Also applies to: 208-210
zetaclient/chains/interfaces/interfaces.go (1)
203-203
: LGTM!The changes to the method name and return type in the
SolanaRPCClient
interface are consistent with the latest terminology used in the Solana ecosystem. The modifications align the method with the updatedsolrpc
package and do not introduce any correctness issues.contrib/localnet/orchestrator/start-zetae2e.sh (4)
94-97
: LGTM!The addition of the funding command for the "v2 ethers" test accounts follows the established pattern in the script. It ensures that the necessary accounts are funded by retrieving the Ethereum address from the configuration file using
yq
and executing a transaction usinggeth
to send 10,000 Ether. The changes align with the existing practices and enhance the script's capability to support the "v2 ethers" testing scenario.
99-102
: LGTM!The addition of the funding command for the "v2 erc20" test accounts follows the established pattern in the script. It ensures that the necessary accounts are funded by retrieving the Ethereum address from the configuration file using
yq
and executing a transaction usinggeth
to send 10,000 Ether. The changes align with the existing practices and enhance the script's capability to support the "v2 erc20" testing scenario.
104-107
: LGTM!The addition of the funding command for the "v2 ethers revert" test accounts follows the established pattern in the script. It ensures that the necessary accounts are funded by retrieving the Ethereum address from the configuration file using
yq
and executing a transaction usinggeth
to send 10,000 Ether. The changes align with the existing practices and enhance the script's capability to support the "v2 ethers revert" testing scenario.
109-112
: LGTM!The addition of the funding command for the "v2 erc20 revert" test accounts follows the established pattern in the script. It ensures that the necessary accounts are funded by retrieving the Ethereum address from the configuration file using
yq
and executing a transaction usinggeth
to send 10,000 Ether. The changes align with the existing practices and enhance the script's capability to support the "v2 erc20 revert" testing scenario.contrib/localnet/docker-compose.yml (1)
230-241
: LGTM!The addition of the
bitcoin-node-sidecar
service to thedocker-compose.yml
file is a valuable enhancement to the Docker setup. The service is properly configured with a build directive pointing to the specific Dockerfile, ensuring that the correct image is built for the sidecar.The service is assigned a container name and hostname of
bitcoin-node-sidecar
, making it easily identifiable within the Docker network. It is connected to themynetwork
network with a static IP address, ensuring a predictable network configuration.The
PORT
environment variable is set to8000
, and the service exposes this port by mapping it to the host's port8000
. This allows communication with the sidecar service from outside the Docker network.Overall, the changes are well-structured and follow best practices for Docker service configuration.
zetaclient/testutils/mocks/solana_rpc.go (1)
Line range hint
138-165
: LGTM!The changes to the
GetLatestBlockhash
method in theSolanaRPCClient
mock are consistent and reflect the updated method name and return type. The code changes are approved..github/workflows/e2e.yml (1)
16-19
: LGTM!The changes to the GitHub Actions workflow configuration are well-structured and improve the usability and efficiency of the workflow:
- Consolidating multiple boolean input parameters into a single
make-targets
input simplifies the input handling.- The updated logic for setting outputs based on the event name and parsing the
make-targets
input enhances the flexibility of the workflow.- The addition of the
V2_MIGRATION_TESTS
output and the timeout for thestart-tss-migration-test
job are reasonable enhancements.The code changes are approved.
Also applies to: 41-41, 95-106, 141-141, 148-150, 216-216
zetaclient/chains/bitcoin/fee.go (3)
24-53
: LGTM!The addition of new constants enhances the fee calculation logic:
defaultTestnetFeeRate
sets a default fee rate for testnet transactions, providing a fallback value.feeRateCountBackBlocks
specifies the number of blocks to consider when estimating fee rates, allowing for flexibility in the estimation process.DynamicDepositorFeeHeightV2
defines the mainnet height from which the dynamic depositor fee V2 will be applied, enabling a smooth upgrade process.These constants improve the handling of testnet fee rates and provide flexibility for future upgrades. The code changes are approved.
258-280
: LGTM!The new
CalcDepositorFeeV2
function introduces an enhanced depositor fee calculation based on the transaction result:
- It handles different network types (regtest, mainnet) appropriately, using default fees for regtest.
- It retrieves the fee rate of the transaction using the
GetTransactionFeeAndRate
function from therpc
package, ensuring accurate fee estimation.- It applies a gas price multiplier to the fee rate, adjusting the fee according to the current network conditions.
- It includes error handling for retrieving the fee rate, enhancing the robustness of the function.
The function provides a more accurate and flexible depositor fee calculation. The code changes are approved.
282-326
: LGTM, but ensure proper usage!The new
GetRecentFeeRate
function retrieves the highest fee rate from recent blocks, specifically designed for testnet usage:
- It includes a check to prevent its use on the mainnet, safeguarding against potential misconfigurations.
- It retrieves the current block number and iterates through recent blocks to compute the highest average fee rate, ensuring the estimation is based on the most relevant data.
- It handles potential errors during the block retrieval and fee rate calculation process.
- If no recent fee rates are available, it defaults to the
defaultTestnetFeeRate
, providing a fallback value.The function provides a reliable fee rate estimation for testnet usage. However, it's crucial to ensure that this function is only used for testnet and not inadvertently called for mainnet.
To verify the proper usage of
GetRecentFeeRate
, consider adding a test case that asserts an error is returned when the function is called with mainnet parameters. Additionally, review the codebase to ensure thatGetRecentFeeRate
is only invoked for testnet scenarios.zetaclient/chains/solana/observer/inbound.go (1)
Line range hint
329-355
: LGTM!The code changes are approved. The updates to the expected number of accounts and the simplified validation logic align with the new requirements for deposit instructions, making the code more concise and maintainable.
e2e/runner/bitcoin.go (1)
185-189
: LGTM!The modifications to the
sendToAddrFromDeployerWithMemo
function are approved. The changes to the function signature, allowing the caller to specify the recipient address, improve its flexibility and reusability. Returning the transaction hash is essential for the new inscription functionality, and the code changes are minimal and straightforward.e2e/config/config.go (2)
64-76
: LGTM!The additions to the
AdditionalAccounts
struct and the corresponding updates to theAsSlice
method are approved. The new fields enhance the struct's capability to manage a broader range of account types, allowing for more diverse testing scenarios. The code changes are straightforward, maintain the existing structure and naming conventions, and ensure seamless integration of the new account types into existing functionality.Also applies to: 232-235
328-343
: LGTM!The updates to the
GenerateKeys
method in theConfig
struct are approved. The initialization of the new account types using thegenerateAccount()
function reflects the expansion of account management functionality, ensuring that the new account types are properly handled. The error handling for each new account generation ensures that any issues during the process are appropriately managed. The code changes follow the existing pattern and style, maintaining consistency and readability.zetaclient/chains/bitcoin/observer/inbound.go (2)
269-269
: LGTM!The code change is approved. Renaming the function call from
GetBtcEvent
toGetBtcEventWithWitness
is a straightforward change that should not introduce any issues.
331-331
: LGTM!The code change is approved. Renaming the function call from
GetBtcEvent
toGetBtcEventWithWitness
is a straightforward change that should not introduce any issues.Makefile (1)
249-257
: LGTM!The addition of the
start-upgrade-v2-migration-test
target is approved. The target is a self-contained addition that sets the necessary environment variables and runs Docker Compose with the appropriate YAML files for executing the v2 migration upgrade test. The changes should not affect existing functionality.cmd/zetae2e/local/local.go (2)
45-45
: LGTM!The addition of the
flagTestV2Migration
flag and its inclusion in theNewLocalCmd
function is approved. The flag allows users to specify whether to run tests related to the migration of v2 contracts, enhancing the testing framework's flexibility.Also applies to: 80-80, 106-106
239-242
: Verify the conditional execution of the v2 migration process.The changes to the
localE2ETest
function introduce a new boolean variabletestV2Migration
that captures the state of theflagTestV2Migration
flag. ThedeployerRunner.RunV2Migration()
function is conditionally executed based on the value oftestV2Migration
, ensuring that the migration process is run only when intended.However, please ensure that the conditional execution is thoroughly tested to confirm that the migration process is triggered correctly when the flag is set and skipped when the flag is not set. Additionally, verify that the updates to the chain parameters in lines 366-368 are not redundantly executed when the migration tests are being run.
Also applies to: 366-368, 370-371
go.mod (2)
46-46
: Verify compatibility and test the updated protocol-contracts version.The version of
github.com/zeta-chain/protocol-contracts
has been updated. Ensure that the updated version is compatible with the current codebase and has been thoroughly tested to avoid any potential issues.
365-365
: Verify compatibility and test the updated ethermint version.The replacement directive for
github.com/evmos/ethermint
has been updated to a newer version. Ensure that the updated version is compatible with the current codebase and has been thoroughly tested to avoid any potential issues.contrib/localnet/scripts/start-zetacored.sh (1)
257-268
: LGTM!The added commands for handling additional user account types in the genesis state setup are well-structured and follow good practices for maintainability and readability.
zetaclient/chains/evm/signer/signer.go (1)
118-121
: LGTM!The modifications to the
SetGatewayAddress
method improve its functionality and thread safety. The use of locking and the clean conversion of the string address to a hexadecimal address usingethcommon.HexToAddress
are good practices for expressiveness and performance.zetaclient/chains/bitcoin/rpc/rpc_live_test.go (11)
81-102
: LGTM!The
getMempoolSpaceTxsByBlock
function is well-structured and follows a clear logic flow. It retrieves the block hash using the provided block number, fetches the mempool.space transactions for the block using the block hash, and returns the relevant data and any errors encountered.
119-154
: LGTM!The
LiveTest_FilterAndParseIncomingTx
test function is comprehensive and covers all the necessary aspects. It sets up a Bitcoin client, retrieves a specific block containing the incoming transaction, filters and parses the incoming transactions using theFilterAndParseIncomingTx
function, and verifies the expected properties of the parsed incoming transaction.
157-181
: LGTM!The
LiveTest_FilterAndParseIncomingTx_Nop
test function is well-structured and covers the necessary aspects. It sets up a Bitcoin client, retrieves a block that contains no incoming transactions, filters and parses the incoming transactions using theFilterAndParseIncomingTx
function, and verifies that the result is empty, indicating no incoming transactions were found.
Line range hint
185-201
: LGTM!The
LiveTest_NewRPCClient
test function is straightforward and covers the necessary aspects. It creates a new Bitcoin RPC client using the provided configuration, retrieves the block count using the RPC client, and verifies that the block count is greater than zero, indicating a successful connection to the Bitcoin node.
Line range hint
204-222
: LGTM!The
LiveTest_GetBlockHeightByHash
test function is well-structured and covers the necessary aspects. It sets up a Bitcoin client, defines the expected block height and the corresponding block hash, tests the error handling by attempting to retrieve the block height using an invalid hash, and retrieves the block height using the valid block hash, verifying that it matches the expected height.
Line range hint
226-294
: LGTM!The
LiveTest_BitcoinFeeRate
test function is comprehensive and covers the necessary aspects. It sets up a Bitcoin client, retrieves the current block count, fetches the fee rates for different block targets using both Conservative and Economical estimation modes, compares the fee rates, and verifies that the Conservative fee rate is greater than or equal to the Economical fee rate for each block target. It also monitors the fee rates every 5 minutes and repeats the comparison process.
Line range hint
351-362
: LGTM!The
LiveTest_AvgFeeRateMainnetMempoolSpace
test function is concise and covers the necessary aspects. It sets up a Bitcoin client, defines the start and end block range for testing, and calls thecompareAvgFeeRate
function to compare the calculated fee rate with mempool.space fee rate for the specified block range.
Line range hint
365-376
: LGTM!The
LiveTest_AvgFeeRateTestnetMempoolSpace
test function is concise and covers the necessary aspects. It sets up a Bitcoin client, defines the start and end block range for testing, and calls thecompareAvgFeeRate
function to compare the calculated fee rate with mempool.space fee rate for the specified block range.
379-387
: LGTM!The
LiveTest_GetRecentFeeRate
test function is straightforward and covers the necessary aspects. It sets up a Bitcoin testnet client, calls theGetRecentFeeRate
function to retrieve the highest fee rate from recent blocks, and verifies that the retrieved fee rate is greater than zero.
454-549
: LGTM!The
LiveTest_GetTransactionFeeAndRate
test function is comprehensive and covers the necessary aspects. It sets up a Bitcoin client, determines the network, calculates the block range for testing, iterates over the blocks, retrieves the mempool.space transactions and corresponding blocks, compares the calculated fee rate with the mempool.space fee rate for each transaction, and verifies that the difference between the calculated fee rate and mempool.space fee rate is within an acceptable threshold.
551-581
: LGTM!The
LiveTest_CalcDepositorFeeV2
test function is well-structured and covers the necessary aspects. It sets up a Bitcoin client, defines a test transaction hash, retrieves the raw transaction result, and tests theCalcDepositorFeeV2
function with different network parameters. It verifies that it returns the default depositor fee for the regression test network and the correct depositor fee for the main network based on the actual fee rate.e2e/e2etests/e2etests.go (1)
76-76
: LGTM!The new constant
TestExtractBitcoinInscriptionMemoName
and the corresponding E2E testTestExtractBitcoinInscriptionMemo
are correctly implemented, enhancing the testing framework to validate the extraction of memos from Bitcoin inscriptions.The changes follow the existing code structure and conventions, making them easy to understand and maintain.
Also applies to: 402-408
changelog.md (1)
Line range hint
3-34
: Changelog for v20.0.0 release looks good!The changelog provides a detailed and well-structured overview of the significant changes introduced in the v20.0.0 release. It covers:
- New features like
MsgUpdateERC20CustodyPauseStatus
,created_timestamp
for cctx status, and Bitcoin inscriptions support.- Refactoring efforts to improve the codebase.
- Addition of end-to-end tests for V2 protocol contracts migration.
- Important fixes addressing issues related to validation, duplicates, and Solana deposit number.
The changelog follows a clear format, making it easy for users and developers to understand the scope and impact of the release.
zetaclient/chains/bitcoin/observer/observer.go (2)
Line range hint
1-1
: LGTM!The removal of the unused
rpc
package import is approved. This change improves code organization and reduces unnecessary dependencies.
630-630
: LGTM!The change to use
bitcoin.GetRecentFeeRate
directly instead ofrpc.GetRecentFeeRate
is approved. This refactoring improves code organization by consolidating related functionality within thebitcoin
package.zetaclient/orchestrator/orchestrator.go (3)
Line range hint
164-184
: LGTM! The changes improve clarity and ensure gateway address synchronization.The updated comments provide better clarity by explicitly mentioning the gateway addresses being updated alongside the zeta connector and ERC20 custody addresses.
The new conditional check for updating the signer's gateway address is a crucial addition. It ensures that the gateway address is always kept in sync with the parameters, which is essential for maintaining correct operational behavior.
392-402
: LGTM! The modified order of operations improves logical flow and prevents inconsistencies.The change in the order of operations within the
runScheduler
method is approved. By moving the retrieval ofcctxList
fromcctxMap
and the update of the pending transactions gauge after the call toresolveSigner
, the code ensures that the signer is resolved before any metrics related to pending transactions are updated.This modification improves the logical flow of the method and prevents potential inconsistencies that could arise if the signer state is not up-to-date when the metrics are updated.
Line range hint
1-1
: Skipped: Learnings not applicable to the current changes.The learnings retrieved from long-term memory provide useful information about the test coverage of the
GetUpdatedSigner
method. However, as this method is not part of the changes in this pull request, no further comments are necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like more than ethermint version change, should we fix PR title, or its wrong commit?
also base branch is develop that already contains ethermint version bump
Yeah base branch was not set correctly |
Description
Backport 3cfbbc2
Summary by CodeRabbit
New Features
make-targets
input parameter.Bug Fixes
Documentation
Chores