refactor: add validations for evm address #2707
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 changes introduce enhancements to the address validation processes across various components of the Zeta deposit functionality. New test cases have been added to ensure invalid addresses are correctly handled, and existing methods have been updated to utilize a more robust validation approach. Additionally, error handling has been improved to ensure that only valid addresses are processed, thereby minimizing the risk of truncation or erroneous processing of invalid address fields. Changes
Assessment against linked issues
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 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2707 +/- ##
===========================================
- Coverage 66.79% 66.54% -0.25%
===========================================
Files 370 371 +1
Lines 20677 20775 +98
===========================================
+ Hits 13811 13825 +14
- Misses 6230 6311 +81
- Partials 636 639 +3
|
lumtis
added
UPGRADE_LIGHT_TESTS
kingpinXD
changed the title
| @@ -203,6 +205,51 @@ func (m CrossChainTx) SetOutboundBallotIndex(index string) { | |||
| } | |||
|
|
|||
| // GetCctxIndexFromBytes returns the CCTX index from a byte array. | |||
| func (m CrossChainTx) GetValidReceiverAddress() (ethcommon.Address, error) { | |||
There was a problem hiding this comment.
Will be adding more unit tests , and wanted to open up this pr for discussions wether this sort of validation based on coin type is the direction we want to go with
|
|
||
| // TODO : Use this function to validate Sender and Receiver address for CCTX | ||
| // https://github.com/zeta-chain/node/issues/2697 | ||
| func ValidateAddressForChain(address string, chainID int64, additionalChains []chains.Chain) error { |
There was a problem hiding this comment.
Same for this , Not using this function right now , but if used eventually it would make make the address validation a bit more strict . Wanted to get some thoughts before adding more unit tests and using this function
kingpinXD
requested review from
brewmaster012,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (3)
x/fungible/types/message_update_system_contract.go (1)
48-50: Add Unit Tests forValidateEthereumAddressUsageThe recent changes to the address validation logic in
message_update_system_contract.godo not appear to be covered by existing unit tests. It is essential to add tests to ensure the robustness of theValidateEthereumAddressfunction, particularly after modifications to its usage.
- Consider creating unit tests that specifically validate the
MsgUpdateSystemContractstruct and its address validation logic.Analysis chain
LGTM! Ensure unit tests cover the new validation logic.
The changes improve the robustness of the address validation process. Ensure that unit tests are added to cover the new validation logic.
Run the following script to verify the test coverage:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new validation logic is covered by tests. # Test: Search for test functions that cover the new validation logic. rg --type go -A 5 $'ValidateEthereumAddress'Length of output: 3660
pkg/address/validate_address.go (1)
48-52: Add Unit Tests forValidateEthereumAddressFunctionThe
ValidateEthereumAddressfunction does not appear to be covered by existing unit tests. It is crucial to add dedicated unit tests to ensure the function behaves as expected and to maintain the robustness of the codebase.
- Consider creating a new test file specifically for testing address validation functions.
- Ensure edge cases and invalid inputs are thoroughly tested.
Analysis chain
LGTM! Ensure unit tests cover the new function.
The function provides robust validation for Ethereum addresses. Ensure that unit tests are added to cover the new function.
Run the following script to verify the test coverage:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new function is covered by tests. # Test: Search for test functions that cover the new function. rg --type go -A 5 $'ValidateEthereumAddress'Length of output: 3660
Script:
#!/bin/bash # Search for test files that might indirectly test the ValidateEthereumAddress function. rg --type go -l 'ValidateEthereumAddress' | grep '_test.go'Length of output: 57
x/crosschain/types/message_whitelist_erc20.go (1)
57-59: Update all instances to use the new validation method.The old method
ethcommon.HexToAddressis still prevalent in the codebase, which may lead to inconsistent address validation. Ensure that all occurrences are updated to useaddress.ValidateEthereumAddressfor uniform and robust validation.
- Files where
ethcommon.HexToAddressis used:
zetaclient/testutils/constant.gozetaclient/testutils/mocks/tss_signer.gozetaclient/orchestrator/bootstrap.gox/fungible/keeper/system_contract.go- And many others as listed in the output.
Analysis chain
LGTM! But verify the usage of the new validation method.
The new validation method and enhanced error handling improve the robustness of the code. Ensure that all usages of the old validation method are updated to use
address.ValidateEthereumAddress.Run the following script to verify the usage of the new validation method:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of the new validation method. # Test: Search for the old validation method. Expect: No occurrences of the old method. rg --type go -A 5 $'ethcommon.HexToAddress' # Test: Search for the new validation method. Expect: Occurrences of the new method. rg --type go -A 5 $'address.ValidateEthereumAddress'Length of output: 63833
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (19)
- cmd/zetae2e/local/local.go (1 hunks)
- e2e/e2etests/e2etests.go (2 hunks)
- e2e/e2etests/test_migrate_chain_support.go (1 hunks)
- e2e/e2etests/test_zeta_deposit.go (2 hunks)
- e2e/e2etests/test_zeta_deposit_new_address.go (1 hunks)
- e2e/e2etests/test_zeta_deposit_restricted_address.go (1 hunks)
- e2e/runner/zeta.go (2 hunks)
- e2e/utils/require.go (1 hunks)
- pkg/address/validate_address.go (1 hunks)
- pkg/address/validate_address_test.go (1 hunks)
- x/crosschain/keeper/cctx_gateway_zevm.go (1 hunks)
- x/crosschain/keeper/evm_deposit.go (1 hunks)
- x/crosschain/keeper/evm_deposit_test.go (2 hunks)
- x/crosschain/types/cctx.go (2 hunks)
- x/crosschain/types/errors.go (1 hunks)
- x/crosschain/types/message_whitelist_erc20.go (2 hunks)
- x/crosschain/types/validate.go (2 hunks)
- x/crosschain/types/validate_test.go (1 hunks)
- x/fungible/types/message_update_system_contract.go (2 hunks)
Files skipped from review due to trivial changes (1)
- x/crosschain/keeper/cctx_gateway_zevm.go
Additional context used
Path-based instructions (18)
e2e/e2etests/test_zeta_deposit_restricted_address.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_zeta_deposit_new_address.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_zeta_deposit.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/types/validate_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/fungible/types/message_update_system_contract.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/address/validate_address.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/types/message_whitelist_erc20.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/utils/require.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/types/validate.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/types/errors.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/keeper/evm_deposit.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/address/validate_address_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/zeta.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_migrate_chain_support.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/types/cctx.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.x/crosschain/keeper/evm_deposit_test.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.
golangci-lint
pkg/address/validate_address.go
6-6: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(github.com/zeta-chain/zetacore)(gci)
GitHub Check: codecov/patch
pkg/address/validate_address.go
[warning] 15-15: pkg/address/validate_address.go#L15
Added line #L15 was not covered by tests
[warning] 29-29: pkg/address/validate_address.go#L29
Added line #L29 was not covered by tests
[warning] 43-44: pkg/address/validate_address.go#L43-L44
Added lines #L43 - L44 were not covered by testsx/crosschain/types/cctx.go
[warning] 208-210: x/crosschain/types/cctx.go#L208-L210
Added lines #L208 - L210 were not covered by tests
[warning] 212-212: x/crosschain/types/cctx.go#L212
Added line #L212 was not covered by tests
[warning] 214-216: x/crosschain/types/cctx.go#L214-L216
Added lines #L214 - L216 were not covered by tests
[warning] 218-218: x/crosschain/types/cctx.go#L218
Added line #L218 was not covered by tests
[warning] 223-223: x/crosschain/types/cctx.go#L223
Added line #L223 was not covered by tests
[warning] 225-228: x/crosschain/types/cctx.go#L225-L228
Added lines #L225 - L228 were not covered by tests
[warning] 230-231: x/crosschain/types/cctx.go#L230-L231
Added lines #L230 - L231 were not covered by tests
[warning] 233-235: x/crosschain/types/cctx.go#L233-L235
Added lines #L233 - L235 were not covered by tests
[warning] 237-237: x/crosschain/types/cctx.go#L237
Added line #L237 was not covered by tests
[warning] 240-240: x/crosschain/types/cctx.go#L240
Added line #L240 was not covered by tests
[warning] 242-244: x/crosschain/types/cctx.go#L242-L244
Added lines #L242 - L244 were not covered by tests
[warning] 246-246: x/crosschain/types/cctx.go#L246
Added line #L246 was not covered by tests
[warning] 248-249: x/crosschain/types/cctx.go#L248-L249
Added lines #L248 - L249 were not covered by tests
Additional comments not posted (24)
e2e/e2etests/test_zeta_deposit_restricted_address.go (1)
18-18: LGTM! But verify the method signature.The change ensures that the address is correctly formatted as a byte slice before being processed. However, ensure that the
DepositZetaWithAmountmethod expects a byte slice and handles it correctly.The code changes are approved.
Run the following script to verify the method signature:
e2e/e2etests/test_zeta_deposit_new_address.go (1)
18-18: LGTM! But verify the method signature.The change ensures that the address is correctly formatted as a byte slice before being processed. However, ensure that the
DepositZetaWithAmountmethod expects a byte slice and handles it correctly.The code changes are approved.
Run the following script to verify the method signature:
e2e/e2etests/test_zeta_deposit.go (2)
27-42: LGTM!The new test function is well-implemented and covers the scenario of depositing to an invalid address. The logging and validation are appropriate.
The code changes are approved.
20-20: LGTM! But verify the method signature.The change ensures that the address is correctly formatted as a byte slice before being processed. However, ensure that the
DepositZetaWithAmountmethod expects a byte slice and handles it correctly.The code changes are approved.
Run the following script to verify the method signature:
pkg/address/validate_address.go (1)
12-46: LGTM! Ensure unit tests cover the new function.The function provides robust validation for addresses across different blockchain networks. Ensure that unit tests are added to cover the new function.
Run the following script to verify the test coverage:
Verification successful
The
ValidateAddressForChainfunction is well-covered by unit tests.The function has comprehensive test coverage, ensuring its reliability across different blockchain networks. The tests include both valid and invalid scenarios, which is essential for robust validation. No further action is required.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new function is covered by tests. # Test: Search for test functions that cover the new function. rg --type go -A 5 $'ValidateAddressForChain'Length of output: 9835
Tools
GitHub Check: codecov/patch
[warning] 15-15: pkg/address/validate_address.go#L15
Added line #L15 was not covered by tests
[warning] 29-29: pkg/address/validate_address.go#L29
Added line #L29 was not covered by tests
[warning] 43-44: pkg/address/validate_address.go#L43-L44
Added lines #L43 - L44 were not covered by testsx/crosschain/types/message_whitelist_erc20.go (1)
7-7: LGTM!The import statement for the
addresspackage is necessary for the new validation method.e2e/utils/require.go (1)
25-34: LGTM!The new function
ContainsStringInCCTXStatusMessageis well-implemented and enhances the testing capabilities.x/crosschain/types/validate.go (2)
5-5: LGTM!The removal of the import statement for
ethcommonis necessary due to the removal of theValidateAddressForChainfunction.
Line range hint
33-33: LGTM! But verify the usage of the new validation method.The removal of the
ValidateAddressForChainfunction indicates a shift in how address validation is handled. Ensure that the new validation method is used consistently across the codebase.Run the following script to verify the usage of the new validation method:
x/crosschain/types/errors.go (1)
59-59: LGTM!The new error variable
ErrInvalidReceiverAddressis correctly registered and follows the existing pattern for error registration.x/crosschain/keeper/evm_deposit.go (1)
27-30: LGTM!The new method call
GetValidReceiverAddressenhances the robustness of the function by ensuring that only valid receiver addresses are processed. The error handling is correctly implemented.pkg/address/validate_address_test.go (1)
1-149: LGTM!The test cases are comprehensive and cover a wide range of scenarios. The use of
requirefrom thetestifypackage ensures that the tests are expressive and fail fast.e2e/runner/zeta.go (2)
98-98: LGTM!The change is consistent with the updated
DepositZetaWithAmountmethod signature.
Line range hint
102-120: LGTM!The change is consistent with the updated method signature and ensures that the
DestinationAddressis correctly passed as a byte slice.e2e/e2etests/test_migrate_chain_support.go (1)
41-41: LGTM!The change is consistent with the updated
DepositZetaWithAmountmethod signature.cmd/zetae2e/local/local.go (1)
253-253: LGTM!The addition of the new test case
TestZetaDepositInvalidAddressNameenhances the robustness of the code by ensuring that edge cases related to address validation are covered.x/crosschain/keeper/evm_deposit_test.go (6)
54-77: LGTM!The test case correctly ensures that invalid receiver addresses are identified and handled for coin type Zeta.
79-103: LGTM!The test case correctly ensures that invalid receiver addresses are identified and handled for coin type Gas.
105-128: LGTM!The test case correctly ensures that invalid receiver addresses are identified and handled for coin type ERC20.
601-601: LGTM!The modification correctly aligns the error handling with the new logic introduced in the added test cases.
129-129: LGTM!The test case correctly ensures that invalid sender addresses are identified and handled.
601-601: LGTM!The test case correctly ensures that failed deposits with non-failed transactions are handled.
e2e/e2etests/e2etests.go (2)
14-14: LGTM!The addition of the constant
TestZetaDepositInvalidAddressNamefollows the existing naming pattern and is correctly implemented.
167-174: LGTM!The new test case
runner.NewE2ETest(TestZetaDepositInvalidAddressName, ...)is correctly added to theAllE2ETestsslice. The test case description and argument definitions are clear and follow the existing pattern.
x/crosschain/types/cctx.go
Outdated
| func (m CrossChainTx) GetValidReceiverAddress() (ethcommon.Address, error) { | ||
| inboundCoinType := m.InboundParams.CoinType | ||
| switch inboundCoinType { | ||
| // For coin type ZETA the receiver is added to the receiver field of the inbound vote so we can use it directly | ||
| case coin.CoinType_Zeta: | ||
| { | ||
| err := address.ValidateEthereumAddress(m.GetCurrentOutboundParam().Receiver) | ||
| if err != nil { | ||
| return ethcommon.Address{}, cosmoserrors.Wrap(ErrInvalidReceiverAddress, err.Error()) | ||
| } | ||
| return ethcommon.HexToAddress(m.GetCurrentOutboundParam().Receiver), nil | ||
| } | ||
| // For coin type gas we create the message by composing the receiver and the call data so we need to parse the address from the RelayedMessage | ||
| // The receiver in this case is set to me the sender of the message | ||
| // For cases where this is not the case we should try and use the receiver field of the inbound vote | ||
| case coin.CoinType_Gas: | ||
| { | ||
| to := m.GetCurrentOutboundParam().Receiver | ||
| parsedAddress, _, err := chains.ParseAddressAndData(m.RelayedMessage) | ||
| if err != nil { | ||
| return ethcommon.Address{}, cosmoserrors.Wrap(ErrInvalidReceiverAddress, err.Error()) | ||
| } | ||
| if parsedAddress != (ethcommon.Address{}) { | ||
| to = parsedAddress.String() | ||
| } | ||
| err = address.ValidateEthereumAddress(to) | ||
| if err != nil { | ||
| return ethcommon.Address{}, cosmoserrors.Wrap(ErrInvalidReceiverAddress, err.Error()) | ||
| } | ||
| return ethcommon.HexToAddress(m.GetCurrentOutboundParam().Receiver), nil | ||
| } | ||
| // For coin type ERC20 we can use the receiver field of the inbound vote | ||
| case coin.CoinType_ERC20: | ||
| { | ||
| err := address.ValidateEthereumAddress(m.GetCurrentOutboundParam().Receiver) | ||
| if err != nil { | ||
| return ethcommon.Address{}, cosmoserrors.Wrap(ErrInvalidReceiverAddress, err.Error()) | ||
| } | ||
| return ethcommon.HexToAddress(m.GetCurrentOutboundParam().Receiver), nil | ||
| } | ||
| default: | ||
| return ethcommon.Address{}, fmt.Errorf("invalid coin type %s", inboundCoinType) | ||
| } | ||
| } |
There was a problem hiding this comment.
LGTM! But add unit tests for the new method.
The method is well-structured and handles different coin types appropriately. However, the lack of test coverage is a concern.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 208-210: x/crosschain/types/cctx.go#L208-L210
Added lines #L208 - L210 were not covered by tests
[warning] 212-212: x/crosschain/types/cctx.go#L212
Added line #L212 was not covered by tests
[warning] 214-216: x/crosschain/types/cctx.go#L214-L216
Added lines #L214 - L216 were not covered by tests
[warning] 218-218: x/crosschain/types/cctx.go#L218
Added line #L218 was not covered by tests
[warning] 223-223: x/crosschain/types/cctx.go#L223
Added line #L223 was not covered by tests
[warning] 225-228: x/crosschain/types/cctx.go#L225-L228
Added lines #L225 - L228 were not covered by tests
[warning] 230-231: x/crosschain/types/cctx.go#L230-L231
Added lines #L230 - L231 were not covered by tests
[warning] 233-235: x/crosschain/types/cctx.go#L233-L235
Added lines #L233 - L235 were not covered by tests
[warning] 237-237: x/crosschain/types/cctx.go#L237
Added line #L237 was not covered by tests
[warning] 240-240: x/crosschain/types/cctx.go#L240
Added line #L240 was not covered by tests
[warning] 242-244: x/crosschain/types/cctx.go#L242-L244
Added lines #L242 - L244 were not covered by tests
[warning] 246-246: x/crosschain/types/cctx.go#L246
Added line #L246 was not covered by tests
[warning] 248-249: x/crosschain/types/cctx.go#L248-L249
Added lines #L248 - L249 were not covered by tests
| return combined | ||
| } | ||
|
|
||
| func TestValidateAddressForChain(t *testing.T) { |
There was a problem hiding this comment.
nit: We should document somewhere that this test has to be extended whenever we include a chain with a different address format.
| } | ||
|
|
||
| // DepositZetaWithAmount deposits ZETA on ZetaChain from the ZETA smart contract on EVM with the specified amount | ||
| func (r *E2ERunner) DepositZetaWithAmount(to ethcommon.Address, amount *big.Int) ethcommon.Hash { | ||
| func (r *E2ERunner) DepositZetaWithAmount(to []byte, amount *big.Int) ethcommon.Hash { |
There was a problem hiding this comment.
AFAIK this function is called when the deposit comes from the EVM, it would be a good idea to change the to parameter to to [20]byte.
There was a problem hiding this comment.
Let’s keep the E2E with ethcommon.Address, rather than refactoring to []byte solely to test an invalid EVM address. I suggest we handle this case change separately.
There was a problem hiding this comment.
I think using []byte is fine for now , it keeps the function generic .
We could move to [20]byte , but since this is a helper function for tests , I don't think its needed for now
| } | ||
|
|
||
| // DepositZetaWithAmount deposits ZETA on ZetaChain from the ZETA smart contract on EVM with the specified amount | ||
| func (r *E2ERunner) DepositZetaWithAmount(to ethcommon.Address, amount *big.Int) ethcommon.Hash { | ||
| func (r *E2ERunner) DepositZetaWithAmount(to []byte, amount *big.Int) ethcommon.Hash { |
There was a problem hiding this comment.
Let’s keep the E2E with ethcommon.Address, rather than refactoring to []byte solely to test an invalid EVM address. I suggest we handle this case change separately.
| case chains.Network_eth: | ||
| return ValidateEthereumAddress(address) | ||
| case chains.Network_zeta: | ||
| return nil |
There was a problem hiding this comment.
This should also validate for evm
There was a problem hiding this comment.
This function is not used right now
It would depend on the v2 protocol refactor on how this value is utilised .
Presently for btc inbound ,the receiver is set as FromAddress which would cause this check to fail and disable btc deposits .
Overall we should test this via end to end tests when using this function
| case chains.Network_bsc: | ||
| return ValidateEthereumAddress(address) | ||
| case chains.Network_optimism: | ||
| return nil |
There was a problem hiding this comment.
I think we need to validate by VM type, not by chain.network, all EVM's have the same addr
There was a problem hiding this comment.
Yes, I agree, to add all new check here we would need adding a new VM for Bitcoin (which can make sense as Bitcoin has some scripting functionality) but this would be higher scope than the PR.
I suggest to use VM for check with an exception for Bitcoin networks
There was a problem hiding this comment.
I agree with the fact the function should check based on the VM, which is in fact what this function does internally .
All EVM based chains can use the same ValidateEthereumAddress function.
Adding individual cases for each chain lets us provide a more granular check for each chain , and choose to ignore checks for some chains if we want to.
IMO for checks like these it would be good be more cautious , and keep it at chain level
| @@ -203,6 +205,51 @@ func (m CrossChainTx) SetOutboundBallotIndex(index string) { | |||
| } | |||
|
|
|||
| // GetCctxIndexFromBytes returns the CCTX index from a byte array. | |||
| func (m CrossChainTx) GetValidReceiverAddress() (ethcommon.Address, error) { | |||
There was a problem hiding this comment.
| func (m CrossChainTx) GetValidReceiverAddress() (ethcommon.Address, error) { | |
| func (m CrossChainTx) GetReceiver() (ethcommon.Address, error) { |
| case chains.Network_bsc: | ||
| return ValidateEthereumAddress(address) | ||
| case chains.Network_optimism: | ||
| return nil |
There was a problem hiding this comment.
Yes, I agree, to add all new check here we would need adding a new VM for Bitcoin (which can make sense as Bitcoin has some scripting functionality) but this would be higher scope than the PR.
I suggest to use VM for check with an exception for Bitcoin networks
| err = address.ValidateEthereumAddress(msg.NewSystemContractAddress) | ||
| if err != nil { |
There was a problem hiding this comment.
| err = address.ValidateEthereumAddress(msg.NewSystemContractAddress) | |
| if err != nil { | |
| if err := address.ValidateEthereumAddress(msg.NewSystemContractAddress); err != nil { |
| err = address.ValidateEthereumAddress(msg.Erc20Address) | ||
| if err != nil { |
There was a problem hiding this comment.
| err = address.ValidateEthereumAddress(msg.Erc20Address) | |
| if err != nil { | |
| if err := address.ValidateEthereumAddress(msg.Erc20Address); err != nil { |
| // For coin type gas we create the message by composing the receiver and the call data so we need to parse the address from the RelayedMessage | ||
| // The receiver in this case is set to me the sender of the message | ||
| // For cases where this is not the case we should try and use the receiver field of the inbound vote |
There was a problem hiding this comment.
This logic will be removed for V2 contracts, receiver will always contain the receiver address (which should have always been the case)
| @@ -24,7 +24,10 @@ const InCCTXIndexKey = "inCctxIndex" | |||
| // returns (isContractReverted, err) | |||
| // (true, non-nil) means CallEVM() reverted | |||
| func (k Keeper) HandleEVMDeposit(ctx sdk.Context, cctx *types.CrossChainTx) (bool, error) { | |||
| to := ethcommon.HexToAddress(cctx.GetCurrentOutboundParam().Receiver) | |||
| to, err := cctx.GetValidReceiverAddress() | |||
There was a problem hiding this comment.
ctx.GetCurrentOutboundParam().Receiver will always contains the receiver moving forward, and since it is on ZetaChain, the address should always be EVM format
I think we could implement a function such as #2789 and use it here with receiver address
There was a problem hiding this comment.
THe function is a good idea .
This is really the only change in logic that the PR implements, IMO if we want to change this , it would make sense to discard this pr for now , and revisit this once we have everything related to v2 functional
Description
Closes #2696
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores