feat: integrate standard memo to Bitcoin inbound and E2E tests #3025
Conversation
ws4charlie
added
zetaclient
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3025 +/- ##
===========================================
+ Coverage 64.13% 64.47% +0.33%
===========================================
Files 411 412 +1
Lines 28837 28976 +139
===========================================
+ Hits 18496 18681 +185
+ Misses 9559 9512 -47
- Partials 782 783 +1
|
|
!!!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 |
ws4charlie
requested review from
brewmaster012,
kingpinXD,
lumtis,
skosito,
swift1337 and
fbac
as code owners
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant updates across various components, primarily enhancing Bitcoin deposit functionalities and testing. Key changes include the addition of new test cases, refactoring of existing functions, and the introduction of new methods for handling Bitcoin inbound events. The modifications also address specific issues related to the outbound receiver address for Bitcoin deposits and establish a new standard for parsing inbound data. Overall, the changes improve code quality, expand testing coverage, and enhance the functionality of Bitcoin-related operations. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 43
🧹 Outside diff range and nitpick comments (40)
pkg/memo/fields.go (1)
Line range hint
1-16: Consider enhancing interface documentation.The interface would benefit from additional documentation explaining:
- The rationale behind the asymmetric parameter usage
- The relationship between the methods
- The expected flow of operations (Pack → Unpack → Validate)
Add comprehensive documentation:
// Fields is the interface for memo fields +// The interface provides methods for encoding and decoding memo fields with different +// validation stages. While packing and validation require an operation code (opCode), +// unpacking is performed independently of the operation type, with validation occurring +// as a separate step. type Fields interface {e2e/e2etests/test_bitcoin_deposit.go (2)
Line range hint
1-24: Enhance test coverage for new memo functionality.Given that this PR introduces standard memo support and addresses receiver address issues, consider adding the following test improvements:
- Add explicit validation of the receiver address in the test
- Add test cases for both standard and legacy memo formats
- Include validation for the correct memo parsing
Here's a suggested enhancement:
func TestBitcoinDeposit(r *runner.E2ERunner, args []string) { require.Len(r, args, 1) depositAmount := parseFloat(r, args[0]) r.SetBtcAddress(r.Name, false) txHash := r.DepositBTCWithAmount(depositAmount, nil) // wait for the cctx to be mined cctx := utils.WaitCctxMinedByInboundHash(r.Ctx, txHash.String(), r.CctxClient, r.Logger, r.CctxTimeout) r.Logger.CCTX(*cctx, "deposit") utils.RequireCCTXStatus(r, cctx, crosschaintypes.CctxStatus_OutboundMined) + + // Validate receiver address + require.Equal(r, r.GetAddress(r.Name).String(), cctx.GetReceiver(), + "receiver address should match the intended recipient") } + +// TestBitcoinDepositWithStandardMemo tests deposit with the new standard memo format +func TestBitcoinDepositWithStandardMemo(r *runner.E2ERunner, args []string) { + // Add test implementation for standard memo +} + +// TestBitcoinDepositWithLegacyMemo tests deposit with the legacy memo format +func TestBitcoinDepositWithLegacyMemo(r *runner.E2ERunner, args []string) { + // Add test implementation for legacy memo +}
Line range hint
20-23: Consider adding error case validation.The test currently only validates the happy path where the transaction is mined successfully. Consider adding test cases for error scenarios, such as:
- Invalid memo format
- Incorrect receiver address format
- Transaction revert scenarios
Would you like me to provide example test cases for these scenarios?
e2e/e2etests/test_bitcoin_donation.go (2)
14-14: Add function documentation.Consider adding a documentation comment that describes the test's purpose, parameters, and expected behavior. This will help other developers understand the test's objectives and requirements.
+// TestBitcoinDonation validates that Bitcoin transactions with donation messages +// are processed correctly but do not create cross-chain context data. +// Args: +// - args[0]: donation amount in BTC func TestBitcoinDonation(r *runner.E2ERunner, args []string) {
36-37: Enhance error context for transaction failure.Consider adding more context to the error when the transaction fails to help with debugging.
txHash, err := r.SendToTSSFromDeployerWithMemo(amountTotal, utxos, memo) -require.NoError(r, err) +require.NoError(r, err, "failed to send donation transaction: %v", err)e2e/e2etests/test_bitcoin_std_deposit_and_call_revert.go (1)
13-25: Add function documentation and improve error handling.The test function would benefit from:
- A detailed documentation comment explaining its purpose, parameters, and test scenario
- Constants for expected argument count
- Explicit error handling for amount parsing
Consider applying these improvements:
+// TestBitcoinStdMemoDepositAndCallRevert tests the Bitcoin deposit and call revert functionality +// using standard memo format. It verifies that when calling a non-existing contract, +// the transaction is properly reverted and funds are refunded. +// Args: +// - args[0]: amount of BTC to send func TestBitcoinStdMemoDepositAndCallRevert(r *runner.E2ERunner, args []string) { + const expectedArgCount = 1 - require.Len(r, args, 1) + require.Len(r, args, expectedArgCount, "expected %d argument (amount)", expectedArgCount)e2e/e2etests/test_stress_btc_deposit.go (3)
33-33: Document the purpose of the nil memo parameter.The
nilparameter represents an optional memo, but its purpose and implications aren't clear from the code. Consider adding a comment explaining why no memo is used in the stress test.
Line range hint
14-62: Enhance test coverage for memo scenarios.The stress test currently only tests basic deposits without memos. Consider expanding the test to cover:
- Deposits with standard memos
- Validation of receiver addresses
- Different revert options
Here's a suggested enhancement:
func TestStressBTCDeposit(r *runner.E2ERunner, args []string) { - require.Len(r, args, 2) + require.Len(r, args, 3) depositAmount := parseFloat(r, args[0]) numDeposits := parseInt(r, args[1]) + testMemo := args[2] == "true" // Optional flag to test with memos r.SetBtcAddress(r.Name, false) r.Logger.Print("starting stress test of %d deposits", numDeposits) var eg errgroup.Group for i := 0; i < numDeposits; i++ { i := i - txHash := r.DepositBTCWithAmount(depositAmount, nil) + var memo *memo.InboundMemo + if testMemo { + // Test different memo scenarios in round-robin fashion + switch i % 3 { + case 0: + memo = memo.NewStandardMemo(r.GetAddress()) + case 1: + memo = memo.NewStandardMemoWithRevert(r.GetAddress(), r.GetAddress()) + case 2: + memo = memo.NewDepositAndCallMemo(r.GetAddress(), []byte("test")) + } + } + txHash := r.DepositBTCWithAmount(depositAmount, memo) r.Logger.Print("index %d: starting deposit, tx hash: %s", i, txHash.String())
Line range hint
63-85: Enhance deposit monitoring to validate receiver addresses.The
monitorBTCDepositfunction could be enhanced to validate that the receiver address in the CCTX matches the intended receiver from the memo (when present).Here's a suggested enhancement:
func monitorBTCDeposit(r *runner.E2ERunner, hash *chainhash.Hash, index int, startTime time.Time) error { cctx := utils.WaitCctxMinedByInboundHash(r.Ctx, hash.String(), r.CctxClient, r.Logger, r.ReceiptTimeout) if cctx.CctxStatus.Status != crosschaintypes.CctxStatus_OutboundMined { return fmt.Errorf( "index %d: deposit cctx failed with status %s, message %s, cctx index %s", index, cctx.CctxStatus.Status, cctx.CctxStatus.StatusMessage, cctx.Index, ) } + // Validate receiver address if memo was used + if len(cctx.InboundTxParams.Memo) > 0 { + parsedMemo, err := memo.ParseInboundMemo(cctx.InboundTxParams.Memo) + if err != nil { + return fmt.Errorf("index %d: failed to parse memo: %v", index, err) + } + if parsedMemo.Receiver != cctx.OutboundTxParams.Receiver { + return fmt.Errorf( + "index %d: receiver mismatch, memo: %s, cctx: %s", + index, + parsedMemo.Receiver, + cctx.OutboundTxParams.Receiver, + ) + } + } timeToComplete := time.Since(startTime) r.Logger.Print("index %d: deposit cctx success in %s", index, timeToComplete.String()) return nil }e2e/e2etests/test_bitcoin_deposit_and_call_revert.go (2)
23-25: Consider improving the argument handling and documentation.
- Replace the magic number with a named constant for better maintainability
- Add documentation explaining why DefaultDepositorFee is added to the amount
+const ( + expectedArgCount = 1 + argAmountIndex = 0 +) -require.Len(r, args, 1) -amount := parseFloat(r, args[0]) +require.Len(r, args, expectedArgCount, "Expected amount argument") +// Parse amount and add depositor fee to cover transaction costs +amount := parseFloat(r, args[argAmountIndex])
13-51: Consider extracting common Bitcoin test setup.The test setup logic (Bitcoin address configuration, mining setup, UTXO validation) appears to be common across Bitcoin tests. Consider extracting this into a shared setup helper to improve maintainability and reduce duplication.
Example structure:
func SetupBitcoinTest(r *runner.E2ERunner) func() { r.SetBtcAddress(r.Name, false) stop := r.MineBlocksIfLocalBitcoin() utxos, err := r.ListDeployerUTXOs() require.NoError(r, err) require.NotEmpty(r, utxos) return stop }e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.go (2)
14-14: Add function documentation.Consider adding a documentation comment that explains:
- The purpose of this E2E test
- Expected args format and their purpose
- Expected behavior when the test passes
Example:
// TestBitcoinStdMemoDepositAndCallRevertOtherAddress tests the Bitcoin deposit and call // functionality when targeting a non-existing contract address. It verifies that // the transaction is properly reverted and funds are sent to the specified revert address. // // Args: // - args[0]: Amount of BTC to send in the deposit transaction
24-25: Improve args validation.The current length check doesn't provide context about the expected argument. Consider adding a more descriptive error message.
-require.Len(r, args, 1) +require.Len(r, args, 1, "expected 1 argument: BTC amount to send")zetaclient/zetacore/constant.go (1)
27-29: Consider removing explicit type for consistency.The constant declaration is well-placed and documented. However, for consistency with other constants in this file, consider removing the explicit
uint64type specification as Go will implicitly type the constant correctly.-PostVoteInboundCallOptionsGasLimit uint64 = 1_500_000 +PostVoteInboundCallOptionsGasLimit = 1_500_000e2e/e2etests/test_bitcoin_std_deposit.go (1)
30-39: Use constants for memo configuration values.Magic numbers in the memo configuration should be replaced with named constants for better maintainability and documentation.
+const ( + memoVersion = 0 + memoEncodingFmt = memo.EncodingFmtCompactShort +) memo := &memo.InboundMemo{ Header: memo.Header{ - Version: 0, + Version: memoVersion, - EncodingFmt: memo.EncodingFmtCompactShort, + EncodingFmt: memoEncodingFmt, OpCode: memo.OpCodeDeposit, },pkg/memo/memo.go (1)
54-54: Enhance function documentation for clarity.The current documentation could be more explicit about error conditions and validation behavior. Consider expanding it to detail specific scenarios where nil is returned versus when errors are returned.
-// Returns nil memo if given data can't be decoded as a memo. +// Returns: +// - (nil, error) if header decoding fails or version is invalid +// - (nil, error) if field unpacking or validation fails +// - (*InboundMemo, nil) on successful decode and validationx/crosschain/types/message_vote_inbound.go (1)
Based on the analysis of the codebase, I can now provide a final response:
MaxMessageLength should be moved to chain-specific parameters for better flexibility
The current implementation uses a hardcoded
MaxMessageLengthconstant (10240 bytes) that is only used in message validation. Given that:
- The constant is only referenced in validation and tests
- The crosschain module lacks a parameter management system
- Different chains may have different requirements for message sizes
- The PR introduces new memo support with potential for larger data structures
Recommended approach:
- Create a new parameter system in the crosschain module similar to other modules (emissions, observer)
- Move MaxMessageLength to chain-specific parameters
- Allow different limits for standard memos vs inscriptions
- Add proper parameter validation and update mechanisms
🔗 Analysis chain
Line range hint
15-20: Consider addressing MaxMessageLength TODO in light of new memo support.Given that this PR introduces standard memo support and the PR objectives mention new inscription support allowing for larger inbound data structures, it might be worth addressing the TODO comment about parameterizing
MaxMessageLength. This would provide more flexibility for different chain requirements.Let's check if this constant is referenced elsewhere:
Consider:
- Making this a chain-specific parameter
- Moving it to a configuration that can be adjusted per network
- Adding different limits for different types of messages (standard vs inscription)
Let me gather more context about the usage and validation of this constant.
The first command will show how MaxMessageLength is being used in context, while the second will help identify if there's an existing parameter system for similar limits. This will help determine if we should integrate with an existing parameter system or propose a new one.
Let me check if there's an existing parameter system in the crosschain module that we could leverage.
This will help determine if there's an existing parameter system we can integrate with for managing the message length limits.
Let me check one final aspect - whether there's a params.proto file in the crosschain module that we could extend.
This will help determine if there's an existing parameter management infrastructure in the crosschain module that we can leverage for the message length limit.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for MaxMessageLength usage across the codebase rg "MaxMessageLength" --type goLength of output: 400
Script:
#!/bin/bash # Get more context around the MaxMessageLength usage in the test and validation rg "MaxMessageLength" -B 2 -A 2 --type go # Check if there are any parameters or configuration related files fd "params|config" --type f --extension go | xargs rg "Length|Size|Limit" -B 1 -A 1Length of output: 19879
Script:
#!/bin/bash # Check for parameter-related files in the crosschain module fd "params|config" --type f --extension go --search-path x/crosschain | xargs rg "Params|Config" -B 1 -A 1 # Look for any existing chain-specific configurations ast-grep --pattern 'type Params struct { $$$ }'Length of output: 8557
Script:
#!/bin/bash # Look for params.proto in the crosschain module fd "params.proto" --type f --search-path x/crosschain # Check if there's any existing parameter management code rg "ParamSet|GetParams|SetParams" --type go --search-path x/crosschainLength of output: 570
testutil/sample/crypto.go (1)
63-73: Add documentation and parameter validation.While the implementation is correct, consider these improvements for better maintainability:
- Add godoc explaining the P2WPKH address format and network parameter requirements
- Add validation for nil network params
Apply this diff:
// BtcAddressP2WPKH returns a sample btc P2WPKH address +// +// P2WPKH (Pay-to-Witness-Public-Key-Hash) is a native SegWit address format +// that provides better efficiency and lower fees compared to legacy addresses. +// +// Parameters: +// - t: testing context for assertions +// - net: Bitcoin network parameters (e.g., chaincfg.MainNetParams, chaincfg.TestNet3Params) +// must not be nil func BtcAddressP2WPKH(t *testing.T, net *chaincfg.Params) string { + require.NotNil(t, net, "network parameters must not be nil") + privateKey, err := btcec.NewPrivateKey() require.NoError(t, err)zetaclient/chains/bitcoin/observer/witness.go (3)
Line range hint
16-54: Enhance error handling and logging structure for better observability.Consider improving the error handling and logging structure in
GetBtcEventWithWitnessto facilitate better debugging and monitoring:func GetBtcEventWithWitness( client interfaces.BTCRPCClient, tx btcjson.TxRawResult, tssAddress string, blockNumber uint64, logger zerolog.Logger, netParams *chaincfg.Params, depositorFee float64, ) (*BTCInboundEvent, error) { if len(tx.Vout) < 1 { - logger.Debug().Msgf("no output %s", tx.Txid) + logger.Debug(). + Str("txid", tx.Txid). + Msg("transaction has no outputs") return nil, nil } if len(tx.Vin) == 0 { - logger.Debug().Msgf("no input found for inbound: %s", tx.Txid) + logger.Debug(). + Str("txid", tx.Txid). + Msg("no input found for inbound transaction") return nil, nil } if err := isValidRecipient(tx.Vout[0].ScriptPubKey.Hex, tssAddress, netParams); err != nil { - logger.Debug().Msgf("irrelevant recipient %s for tx %s, err: %s", tx.Vout[0].ScriptPubKey.Hex, tx.Txid, err) + logger.Debug(). + Str("txid", tx.Txid). + Str("script", tx.Vout[0].ScriptPubKey.Hex). + Err(err). + Msg("irrelevant recipient for transaction") return nil, nil }
Line range hint
133-142: Improve error handling and function structure intryExtractOpRet.The function could benefit from a more consistent error handling pattern and better readability.
func tryExtractOpRet(tx btcjson.TxRawResult, logger zerolog.Logger) []byte { if len(tx.Vout) < 2 { - logger.Debug().Msgf("txn %s has fewer than 2 outputs, not target OP_RETURN txn", tx.Txid) + logger.Debug(). + Str("txid", tx.Txid). + Int("outputs", len(tx.Vout)). + Msg("insufficient outputs for OP_RETURN transaction") return nil } memo, found, err := bitcoin.DecodeOpReturnMemo(tx.Vout[1].ScriptPubKey.Hex) if err != nil { - logger.Error().Err(err).Msgf("tryExtractOpRet: error decoding OP_RETURN memo: %s", tx.Vout[1].ScriptPubKey.Hex) + logger.Error(). + Err(err). + Str("txid", tx.Txid). + Str("script", tx.Vout[1].ScriptPubKey.Hex). + Msg("failed to decode OP_RETURN memo") return nil } - if found { - return memo - } - return nil + return memo if found else nil
Line range hint
71-89: Enhance documentation for Bitcoin-specific logic inParseScriptFromWitness.The function handles complex Bitcoin witness parsing logic that would benefit from more detailed documentation, especially regarding the BIP341 implementation.
+// ParseScriptFromWitness attempts to parse the script from witness data according to BIP341. +// The function handles the following cases: +// 1. Empty witness data +// 2. Annex detection and removal (0x50 byte at the start of last element) +// 3. Script path spending validation +// +// Parameters: +// - witness: Array of hex-encoded witness elements +// - logger: Logger instance for debug information +// +// Returns: +// - []byte: Decoded script if found, nil otherwise func ParseScriptFromWitness(witness []string, logger zerolog.Logger) []byte {pkg/memo/fields_v0.go (1)
Line range hint
171-173: Consider optimizing abort address handling.The conversion of
abortAddressto string viaHex()after unpacking could be optimized since it was originally a string in theRevertOptions.Consider maintaining the original string format:
-var abortAddress common.Address -if zetabits.IsBitSet(dataFlags, bitPosAbortAddress) { - codec.AddArguments(ArgAbortAddress(&abortAddress)) +if zetabits.IsBitSet(dataFlags, bitPosAbortAddress) { + var tmpAddr common.Address + codec.AddArguments(ArgAbortAddress(&tmpAddr)) + f.RevertOptions.AbortAddress = tmpAddr.Hex() } - -// convert abort address to string -if !crypto.IsEmptyAddress(abortAddress) { - f.RevertOptions.AbortAddress = abortAddress.Hex() -}pkg/memo/memo_test.go (1)
158-158: Consider a more descriptive field name.The field name
invalidFieldcould be more explicit about its purpose. Consider renaming it toexpectMemoOnValidationFailureorreturnMemoOnInvalidFieldto better convey its role in the test cases.pkg/memo/fields_v0_test.go (1)
Line range hint
1-400: Consider adding test cases for Bitcoin-specific scenarios.Given that this PR focuses on Bitcoin inbound transactions and standard memo functionality, consider adding test cases that specifically validate:
- Bitcoin address handling in receiver and revert addresses
- Memo size constraints for Bitcoin transactions
- Edge cases around inscription data structures
Here's a suggested test case to add:
{ name: "bitcoin address validation", encodeFmt: memo.EncodingFmtABI, dataFlags: flagsAllFieldsSet, data: ABIPack(t, memo.ArgReceiver(fAddress), memo.ArgPayload(fBytes), memo.ArgRevertAddress("bc1qxy2kgdygjrsqtzq2n0yrf2493p83kkfjhx0wlh"), // Bitcoin address memo.ArgAbortAddress(fAddress), memo.ArgRevertMessage(fBytes)), errMsg: "invalid address format for Bitcoin network", },x/crosschain/types/message_vote_inbound_test.go (2)
45-82: Enhance test coverage with edge cases and negative scenariosThe test case is well-structured and covers the basic functionality of RevertOptions. Consider adding the following test scenarios to make it more robust:
- Edge cases:
- Maximum gas limit
- Empty revert message
- Invalid addresses
- Negative scenarios:
- Zero gas limit
- Invalid hex addresses
Here's a suggested addition to strengthen the test coverage:
t.Run("handles edge cases for revert options", func(t *testing.T) { tests := []struct { name string options types.RevertOptions wantErr bool }{ { name: "max gas limit", options: types.RevertOptions{ RevertAddress: sample.EthAddress().Hex(), RevertGasLimit: math.NewUintFromString("115792089237316195423570985008687907853269984665640564039457584007913129639935"), }, }, { name: "invalid address format", options: types.RevertOptions{ RevertAddress: "invalid", }, wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { msg := types.NewMsgVoteInbound( // ... other required fields ... types.WithRevertOptions(tt.options), ) err := msg.ValidateBasic() if tt.wantErr { require.Error(t, err) return } require.NoError(t, err) }) } })
Line range hint
1-1: Add package documentation for test coverageConsider adding a package comment to document the test coverage strategy and important test scenarios. This helps maintainers understand the test organization and identify gaps in coverage.
Add the following documentation at the beginning of the file:
/* Package types_test provides comprehensive testing for MsgVoteInbound message handling. Test coverage includes: - Basic validation - Revert options handling - Cross-chain call options - Message digest computation - Signer validation - Protocol versioning */ package types_testcmd/zetae2e/local/local.go (1)
Modularization recommendation validated: File requires immediate structural improvements
Based on the analysis:
- The file is significantly large (515 lines) with multiple test routines
- There are 10 distinct test routines covering different aspects (Bitcoin, Ethereum, Solana, etc.)
- An existing TODO comment explicitly acknowledges the need for simplification
- No similar modularized test files exist yet, indicating this would be a pioneering improvement
Recommended modularization approach:
- Create a dedicated
testcasesdirectory undercmd/zetae2e/local/- Split test routines into focused files (e.g.,
bitcoin_tests.go,ethereum_tests.go)- Maintain a common test configuration and utilities in a shared package
This restructuring will:
- Reduce cognitive load (current file is 515 lines)
- Enable parallel development of test suites
- Improve maintainability and test isolation
- Set a precedent for future test organization
🔗 Analysis chain
Line range hint
102-102: Consider expediting the modularization of test routines.The TODO comment correctly identifies the need to simplify this file. Given the addition of new test cases and growing complexity, consider prioritizing the modularization effort to:
- Improve maintainability by separating test routines into dedicated files
- Enhance readability by reducing file size
- Make it easier to add new test cases in the future
Let's analyze the current file size and complexity:
Let me gather more context about the test routines and their complexity before making the final recommendation.
This will help us understand:
- The complexity of each test routine
- Any existing modularization plans
- Similar test file organization patterns in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze file complexity to support modularization recommendation # Check file size and number of functions echo "File size and function count:" wc -l cmd/zetae2e/local/local.go rg -c "^func" cmd/zetae2e/local/local.go # List all test routines that could be moved to separate files echo -e "\nTest routines that could be modularized:" rg "TestRoutine" cmd/zetae2e/local/local.goLength of output: 1088
Script:
#!/bin/bash # Description: Analyze test routine implementations and their complexity # Find the implementation of test routines echo "Test routine implementations:" ast-grep --pattern 'func $_TestRoutine($$$) { $$$ }' # Check TODO comments related to modularization echo -e "\nTODO comments about modularization:" rg -i "todo.*modular|todo.*simplif|todo.*refactor" cmd/zetae2e/local/local.go # Check if there are similar test files in the codebase echo -e "\nSimilar test files:" fd -e go -e test.go zetae2eLength of output: 456
zetaclient/chains/bitcoin/tx_script_test.go (1)
370-411: LGTM! Well-structured table-driven tests for error cases.The table-driven tests provide comprehensive coverage of error scenarios with clear test names and expected error messages.
Consider adding a constant for the common hex prefix "6a" (OP_RETURN) to improve maintainability:
+const ( + opReturn = "6a" +) + tests := []struct { name string scriptHex string errMsg string }{ { name: "should return error on invalid hex", - scriptHex: "6a14xy", + scriptHex: opReturn + "14xy", errMsg: "error decoding script hex", }, // ... rest of the test cases }e2e/e2etests/e2etests.go (1)
75-91: Consider adding documentation comments for new test constants.While the constant names are descriptive, adding documentation comments would improve maintainability by explicitly stating the purpose and expected behavior of each new test, especially for the standard memo-related tests.
Apply this diff to add documentation:
+// Bitcoin deposit tests with standard memo support TestBitcoinStdMemoDepositName = "bitcoin_std_memo_deposit" TestBitcoinStdMemoDepositAndCallName = "bitcoin_std_memo_deposit_and_call" TestBitcoinStdMemoDepositAndCallRevertName = "bitcoin_std_memo_deposit_and_call_revert" TestBitcoinStdMemoDepositAndCallRevertOtherAddressName = "bitcoin_std_memo_deposit_and_call_revert_other_address"changelog.md (1)
24-24: Enhance the changelog entry with more details.The current entry "standard memo for Bitcoin inbound" could be more descriptive to better communicate the impact and scope of the change.
Consider expanding the entry to:
-* [3025](https://github.com/zeta-chain/node/pull/3025) - standard memo for Bitcoin inbound +* [3025](https://github.com/zeta-chain/node/pull/3025) - add support for standard memo format in Bitcoin inbound transactions, enabling explicit function calls and receiver address specificationzetaclient/chains/bitcoin/observer/event.go (2)
139-140: Increase test coverage for error handling in legacy memo decodingLines 139-140 handle the error case when decoding a legacy memo fails. Currently, this branch is not covered by tests.
Consider adding unit tests that provide invalid legacy memo bytes to
DecodeEventMemoBytes. This will ensure that the error handling logic is verified and robust against malformed input.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 139-140: zetaclient/chains/bitcoin/observer/event.go#L139-L140
Added lines #L139 - L140 were not covered by tests
173-173: Ensure all code paths are tested inCheckEventProcessabilityLine 173 contains the default return statement of
true, which is executed if none of the switch cases match. This line is not covered by tests.After handling the unexpected
InboundProcessabilityvalues with adefaultcase as suggested, update the tests to cover this new branch. This will improve the test coverage and ensure that all possible code paths are verified.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 173-173: zetaclient/chains/bitcoin/observer/event.go#L173
Added line #L173 was not covered by testse2e/utils/zetacore.go (1)
207-225: Enhance Function DocumentationThe
WaitCctxRevertedByInboundHashfunction lacks descriptive documentation. Providing clear comments will improve maintainability and help other developers understand its purpose and usage.Consider adding a detailed comment:
// WaitCctxRevertedByInboundHash waits until a cross-chain transaction (CCTX) with the specified inbound hash // is reverted. It utilizes a match function to filter CCTXs based on defined criteria and returns the first // matching CCTX found within the context's deadline.zetaclient/chains/bitcoin/observer/event_test.go (3)
27-42: Enhance Documentation forcreateTestBtcEventFunctionConsider providing more detailed documentation for the
createTestBtcEventfunction. Including parameter descriptions and explaining the purpose of each field in the returnedBTCInboundEventstruct will improve code readability and assist future maintainers.
137-228: Improve Test Data Clarity by GeneratingMemoBytesProgrammaticallyThe test cases in
Test_DecodeEventMemoBytesutilize hard-coded hex strings forMemoBytes(e.g., lines 143, 163, 179, etc.). Generating these memo bytes programmatically or adding comments that explain the composition of these hex strings can enhance the readability and maintainability of the tests. This approach makes it easier for others to understand and modify the test cases as needed.
344-346: Add Nil Check forvoteBefore AssertionIn
Test_NewInboundVoteV1, after obtainingvotefromob.NewInboundVoteV1(event, amountSats)(line 344), consider adding a nil check before proceeding to the assertion on line 345. This ensures that a nilvotedoes not cause a panic, improving the robustness of the test.zetaclient/chains/bitcoin/observer/inbound.go (1)
140-144: Increase Unit Test Coverage for New CodeThe newly added code lines are not covered by unit tests. To ensure reliability and prevent future regressions, consider adding unit tests for these sections.
Also applies to: 282-282, 379-379
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 140-140: zetaclient/chains/bitcoin/observer/inbound.go#L140
Added line #L140 was not covered by tests
[warning] 142-142: zetaclient/chains/bitcoin/observer/inbound.go#L142
Added line #L142 was not covered by tests
[warning] 144-144: zetaclient/chains/bitcoin/observer/inbound.go#L144
Added line #L144 was not covered by testszetaclient/chains/evm/observer/v2_inbound.go (3)
Line range hint
185-210: Add Unit Tests fornewDepositAndCallInboundVoteFunctionThe
newDepositAndCallInboundVotefunction is a critical addition for handling deposit and call events. To ensure correctness and prevent future regressions, it's advisable to add unit tests that cover various scenarios, including different asset types and event payloads.Would you like assistance in generating unit tests for this function?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 195-195: zetaclient/chains/evm/observer/v2_inbound.go#L195
Added line #L195 was not covered by tests
Line range hint
242-282: Refactor Observation Methods to Reduce Code DuplicationThe
ObserveGatewayDepositAndCallmethod shares significant similarities withObserveGatewayDepositandObserveGatewayCall. Extracting common logic into shared helper functions or utilizing interfaces can improve maintainability and reduce code duplication.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 195-195: zetaclient/chains/evm/observer/v2_inbound.go#L195
Added line #L195 was not covered by tests
Line range hint
284-320: Consolidate Event Parsing Functions for MaintainabilityThe
parseAndValidateDepositAndCallEventsfunction closely resemblesparseAndValidateDepositEventsandparseAndValidateCallEvents. Refactoring these methods to share common code can reduce redundancy and simplify future updates or bug fixes.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 195-195: zetaclient/chains/evm/observer/v2_inbound.go#L195
Added line #L195 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (33)
- changelog.md (1 hunks)
- cmd/zetae2e/local/local.go (1 hunks)
- e2e/e2etests/e2etests.go (2 hunks)
- e2e/e2etests/test_bitcoin_deposit.go (1 hunks)
- e2e/e2etests/test_bitcoin_deposit_and_call_revert.go (1 hunks)
- e2e/e2etests/test_bitcoin_deposit_refund.go (0 hunks)
- e2e/e2etests/test_bitcoin_donation.go (1 hunks)
- e2e/e2etests/test_bitcoin_std_deposit.go (1 hunks)
- e2e/e2etests/test_bitcoin_std_deposit_and_call.go (1 hunks)
- e2e/e2etests/test_bitcoin_std_deposit_and_call_revert.go (1 hunks)
- e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.go (1 hunks)
- e2e/e2etests/test_stress_btc_deposit.go (1 hunks)
- e2e/runner/bitcoin.go (6 hunks)
- e2e/utils/zetacore.go (1 hunks)
- pkg/memo/fields.go (1 hunks)
- pkg/memo/fields_v0.go (1 hunks)
- pkg/memo/fields_v0_test.go (1 hunks)
- pkg/memo/memo.go (2 hunks)
- pkg/memo/memo_test.go (3 hunks)
- tests/simulation/sim/sim_state.go (1 hunks)
- testutil/helpers.go (2 hunks)
- testutil/sample/crypto.go (2 hunks)
- x/crosschain/types/message_vote_inbound.go (1 hunks)
- x/crosschain/types/message_vote_inbound_test.go (1 hunks)
- zetaclient/chains/bitcoin/observer/event.go (1 hunks)
- zetaclient/chains/bitcoin/observer/event_test.go (1 hunks)
- zetaclient/chains/bitcoin/observer/inbound.go (5 hunks)
- zetaclient/chains/bitcoin/observer/inbound_test.go (2 hunks)
- zetaclient/chains/bitcoin/observer/witness.go (1 hunks)
- zetaclient/chains/bitcoin/tx_script.go (1 hunks)
- zetaclient/chains/bitcoin/tx_script_test.go (1 hunks)
- zetaclient/chains/evm/observer/v2_inbound.go (2 hunks)
- zetaclient/zetacore/constant.go (1 hunks)
💤 Files with no reviewable changes (1)
- e2e/e2etests/test_bitcoin_deposit_refund.go
🧰 Additional context used
📓 Path-based instructions (31)
cmd/zetae2e/local/local.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/e2etests.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_bitcoin_deposit.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_bitcoin_deposit_and_call_revert.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_bitcoin_donation.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_bitcoin_std_deposit.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_bitcoin_std_deposit_and_call.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_bitcoin_std_deposit_and_call_revert.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_stress_btc_deposit.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/utils/zetacore.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/memo/fields.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/memo/fields_v0.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/memo/fields_v0_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/memo/memo.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/memo/memo_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.tests/simulation/sim/sim_state.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.testutil/helpers.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.testutil/sample/crypto.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/types/message_vote_inbound.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/types/message_vote_inbound_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/event.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/observer/event_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/inbound.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_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.zetaclient/chains/bitcoin/tx_script.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/tx_script_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/zetacore/constant.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
📓 Learnings (2)
zetaclient/chains/bitcoin/observer/event.go (4)
Learnt from: ws4charlie PR: zeta-chain/node#2899 File: zetaclient/chains/bitcoin/observer/inbound.go:37-38 Timestamp: 2024-09-19T18:22:35.964Z Learning: In `BTCInboundEvent`, it's acceptable to use `float64` for monetary values (`Value` and `DepositorFee`) because precision is ensured through conversion to integer when building the vote message.Learnt from: ws4charlie PR: zeta-chain/node#2899 File: zetaclient/chains/bitcoin/observer/inbound.go:37-38 Timestamp: 2024-10-08T15:34:48.217Z Learning: In `BTCInboundEvent`, it's acceptable to use `float64` for monetary values (`Value` and `DepositorFee`) because precision is ensured through conversion to integer when building the vote message.Learnt from: ws4charlie PR: zeta-chain/node#2899 File: zetaclient/chains/bitcoin/observer/inbound.go:37-38 Timestamp: 2024-10-08T15:34:48.217Z Learning: In `BTCInboundEvent`, it's acceptable to use `float64` for monetary values (`Value` and `DepositorFee`) because precision is ensured through conversion to integer when building the vote message.Learnt from: ws4charlie PR: zeta-chain/node#2899 File: zetaclient/chains/bitcoin/observer/inbound.go:37-38 Timestamp: 2024-10-08T15:34:47.578Z Learning: In `BTCInboundEvent`, it's acceptable to use `float64` for monetary values (`Value` and `DepositorFee`) because precision is ensured through conversion to integer when building the vote message.zetaclient/chains/bitcoin/observer/inbound.go (8)
Learnt from: ws4charlie PR: zeta-chain/node#2899 File: zetaclient/chains/bitcoin/observer/inbound.go:131-132 Timestamp: 2024-10-08T15:34:47.578Z Learning: ObserveInbound coverage will be improved in future refactor.Learnt from: ws4charlie PR: zeta-chain/node#2899 File: zetaclient/chains/bitcoin/observer/inbound.go:131-132 Timestamp: 2024-10-08T15:34:48.217Z Learning: ObserveInbound coverage will be improved in future refactor.Learnt from: ws4charlie PR: zeta-chain/node#2899 File: zetaclient/chains/bitcoin/observer/inbound.go:131-132 Timestamp: 2024-10-08T15:34:48.217Z Learning: ObserveInbound coverage will be improved in future refactor.Learnt from: ws4charlie PR: zeta-chain/node#2899 File: zetaclient/chains/bitcoin/observer/inbound.go:131-132 Timestamp: 2024-09-19T18:25:57.534Z Learning: ObserveInbound coverage will be improved in future refactor.Learnt from: ws4charlie PR: zeta-chain/node#2899 File: zetaclient/chains/bitcoin/observer/inbound.go:37-38 Timestamp: 2024-10-08T15:34:47.578Z Learning: In `BTCInboundEvent`, it's acceptable to use `float64` for monetary values (`Value` and `DepositorFee`) because precision is ensured through conversion to integer when building the vote message.Learnt from: ws4charlie PR: zeta-chain/node#2899 File: zetaclient/chains/bitcoin/observer/inbound.go:37-38 Timestamp: 2024-09-19T18:22:35.964Z Learning: In `BTCInboundEvent`, it's acceptable to use `float64` for monetary values (`Value` and `DepositorFee`) because precision is ensured through conversion to integer when building the vote message.Learnt from: ws4charlie PR: zeta-chain/node#2899 File: zetaclient/chains/bitcoin/observer/inbound.go:37-38 Timestamp: 2024-10-08T15:34:48.217Z Learning: In `BTCInboundEvent`, it's acceptable to use `float64` for monetary values (`Value` and `DepositorFee`) because precision is ensured through conversion to integer when building the vote message.Learnt from: ws4charlie PR: zeta-chain/node#2899 File: zetaclient/chains/bitcoin/observer/inbound.go:37-38 Timestamp: 2024-10-08T15:34:48.217Z Learning: In `BTCInboundEvent`, it's acceptable to use `float64` for monetary values (`Value` and `DepositorFee`) because precision is ensured through conversion to integer when building the vote message.
🪛 GitHub Check: codecov/patch
tests/simulation/sim/sim_state.go
[warning] 251-251: tests/simulation/sim/sim_state.go#L251
Added line #L251 was not covered by testszetaclient/chains/bitcoin/observer/event.go
[warning] 139-140: zetaclient/chains/bitcoin/observer/event.go#L139-L140
Added lines #L139 - L140 were not covered by tests
[warning] 173-173: zetaclient/chains/bitcoin/observer/event.go#L173
Added line #L173 was not covered by testszetaclient/chains/bitcoin/observer/inbound.go
[warning] 140-140: zetaclient/chains/bitcoin/observer/inbound.go#L140
Added line #L140 was not covered by tests
[warning] 142-142: zetaclient/chains/bitcoin/observer/inbound.go#L142
Added line #L142 was not covered by tests
[warning] 144-144: zetaclient/chains/bitcoin/observer/inbound.go#L144
Added line #L144 was not covered by tests
[warning] 282-282: zetaclient/chains/bitcoin/observer/inbound.go#L282
Added line #L282 was not covered by tests
[warning] 379-379: zetaclient/chains/bitcoin/observer/inbound.go#L379
Added line #L379 was not covered by testszetaclient/chains/bitcoin/tx_script.go
[warning] 204-204: zetaclient/chains/bitcoin/tx_script.go#L204
Added line #L204 was not covered by testszetaclient/chains/evm/observer/v2_inbound.go
[warning] 195-195: zetaclient/chains/evm/observer/v2_inbound.go#L195
Added line #L195 was not covered by tests
[warning] 331-331: zetaclient/chains/evm/observer/v2_inbound.go#L331
Added line #L331 was not covered by tests
🔇 Additional comments (34)
pkg/memo/fields.go (1)
9-9: Verify the asymmetric handling of opCode parameter.The
Unpackmethod no longer requiresopCodewhilePackandValidatemethods still use it. This asymmetry might indicate:
- OpCode validation is handled elsewhere in the call chain
- OpCode is not needed during unpacking
- Potential inconsistency in the interface design
Let's verify the usage pattern:
✅ Verification successful
Let me search for more specific information about the memo package implementation to understand the asymmetry better.
Let me search for the actual implementation details of the Unpack method to understand how it handles the opCode parameter.
The asymmetric handling of opCode is intentionally designed and correct.
The code analysis reveals a well-structured design where:
Packrequires opCode to encode the memo fields with the correct operation typeValidateneeds opCode to enforce operation-specific rules (e.g., payload restrictions for OpCodeDeposit)Unpackdoesn't need opCode because:
- Operation type is already encoded in the memo header
- Validation of operation-specific rules happens during Pack/Validate
This asymmetry aligns with the single responsibility principle:
- Encoding (Pack): Needs full context to create the memo
- Validation: Enforces operation-specific rules
- Decoding (Unpack): Simply extracts data using the format specified in the header
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Unpack method implementations and their callers echo "Searching for Unpack implementations:" ast-grep --pattern 'func ($x *$t) Unpack($$$) $$$' echo -e "\nSearching for Unpack method calls:" rg -A 2 '\.Unpack\('Length of output: 43134
Script:
#!/bin/bash # Search for the complete interface definition and its implementations echo "Searching for memo Fields interface definition:" rg -A 10 "type Fields interface" pkg/memo/ echo -e "\nSearching for Fields implementations:" ast-grep --pattern 'type $x struct { $$$ }' pkg/memo/ echo -e "\nSearching for Pack method implementations:" ast-grep --pattern 'func ($x *$t) Pack($$$) $$$' pkg/memo/Length of output: 1060
Script:
#!/bin/bash # Search for Unpack implementations and related code echo "Searching for Unpack implementations:" rg -A 5 "func.*Unpack.*\(" pkg/memo/ echo -e "\nSearching for opCode usage in memo package:" rg "opCode" pkg/memo/ echo -e "\nSearching for files in memo package:" fd . pkg/memo/Length of output: 4563
e2e/e2etests/test_bitcoin_deposit.go (1)
18-18: Verify the signature change impact across the codebase.The updated call to
DepositBTCWithAmountwith anilparameter appears to be part of the new standard memo integration. Let's verify this change is consistent across the codebase.✅ Verification successful
Based on the results, I can now generate a final response as we have sufficient information about the signature changes and their usage across the codebase.
Signature change is consistently implemented across the codebase
The verification shows that:
- The
DepositBTCWithAmountfunction ine2e/runner/bitcoin.gocorrectly accepts two parameters:amount float64andmemo *memo.InboundMemo- All calls to this function across the codebase have been updated to use the new signature:
- Standard deposit tests pass
memoobject:test_bitcoin_std_deposit.go,test_bitcoin_std_deposit_and_call.go- Basic deposit tests pass
nilfor memo:test_bitcoin_deposit.go,test_stress_btc_deposit.go- Revert scenario tests properly utilize the memo parameter:
test_bitcoin_std_deposit_and_call_revert.go,test_bitcoin_std_deposit_and_call_revert_other_address.go🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining calls to the old signature # and verify the implementation of the new signature # Search for all calls to DepositBTCWithAmount rg -A 2 "DepositBTCWithAmount\(" # Search for the implementation to verify parameter handling ast-grep --pattern 'func $_($$) DepositBTCWithAmount($_, $_) { $$$ }'Length of output: 1751
testutil/helpers.go (1)
4-11: LGTM: Import organization follows Go conventionsThe imports are properly organized with standard library imports grouped separately from external dependencies.
e2e/e2etests/test_bitcoin_std_deposit_and_call_revert.go (1)
1-11: LGTM! Clean and well-organized imports.The imports are properly organized with standard library packages separated from project-specific ones.
e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.go (1)
46-46: Verify error handling for deposit transaction.Consider adding test cases to verify error scenarios, such as:
- Invalid amount
- Malformed memo
- Network issues
zetaclient/zetacore/constant.go (1)
27-29: Verify gas limit value usage and sufficiency.The gas limit of 1,500,000 appears reasonable as it:
- Matches
PostGasPriceGasLimit- Is higher than basic operations (200,000)
- Is lower than complex EVM interactions (4,000,000 - 7,000,000)
Let's verify its usage and adequacy:
✅ Verification successful
Based on the search results, I can now generate a final response as we have sufficient information about the gas limit usage and context.
Gas limit value of 1,500,000 is appropriately configured
The constant
PostVoteInboundCallOptionsGasLimitis:
- Used specifically in inbound vote processing (
v2_inbound.go)- Well-positioned within the range of other gas limits in the codebase:
- Higher than basic operations (21,000 for simple transfers)
- Lower than complex operations (test cases show up to 1,000,000)
- Consistent with the codebase's gas management patterns, where gas limits are explicitly set and validated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage and context of the new gas limit constant # Test 1: Find where this constant is used echo "Searching for PostVoteInboundCallOptionsGasLimit usage:" rg "PostVoteInboundCallOptionsGasLimit" # Test 2: Look for similar gas consumption patterns in call options echo -e "\nSearching for gas consumption patterns in call options:" rg -i "call.*options.*gas"Length of output: 8504
e2e/e2etests/test_bitcoin_std_deposit.go (1)
44-47: Enhance CCTX verification.Consider adding more comprehensive checks for the CCTX:
- Verify the receiver address matches the memo
- Validate the amount in the CCTX
- Check for any fees deducted
pkg/memo/memo.go (2)
67-71: LGTM! Clean error handling with good context.The separation of unpacking and validation concerns improves code maintainability.
Line range hint
54-80: Verify memo decoding usage in Bitcoin inbound processing.Given this change affects Bitcoin inbound data processing, let's verify all callers handle nil memo returns correctly.
✅ Verification successful
Let me gather more context about how the nil memo is handled in the Bitcoin observer.
Let me try one more verification to get the complete Bitcoin observer implementation.
Based on the context we've gathered, I can now provide a final response about the memo decoding usage in Bitcoin processing.
✓ Bitcoin observer correctly handles nil memo returns
The verification shows that the Bitcoin observer implementation in
zetaclient/chains/bitcoin/observer/event.goproperly handles the nil memo case through a switch statement pattern:memoStd, err = memo.DecodeFromBytes(event.MemoBytes) switch { case memoStd != nil: // Handles valid memo with additional validationThe code structure ensures that nil memos are safely handled by falling through the switch statement, while valid memos undergo proper validation including:
- Checking for improper data in valid memos
- Validating operation codes (specifically blocking OpCodeCall)
- Verifying revert address validity
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for DecodeFromBytes usage in Bitcoin inbound processing rg -A 5 "DecodeFromBytes\(" | grep -B 5 -A 5 "bitcoin"Length of output: 656
Script:
#!/bin/bash # Get more context around the Bitcoin observer's memo handling ast-grep --pattern 'memoStd, err = memo.DecodeFromBytes($_)' -A 20Length of output: 1779
Script:
#!/bin/bash # Check the full Bitcoin observer event processing implementation rg -l "bitcoin.*observer.*event" | xargs catLength of output: 46
Script:
#!/bin/bash # Get the Bitcoin observer implementation using a more specific pattern rg -A 10 "type.*BitcoinObserver"Length of output: 34
Script:
#!/bin/bash # Find and show the event handling function ast-grep --pattern 'func ($_ *BitcoinObserver) $_($_, $_, $_) error { $$$ }'Length of output: 80
x/crosschain/types/message_vote_inbound.go (1)
25-30: LGTM: Clean implementation of revert options setter.The implementation follows the established pattern of option setters in the codebase and maintains good consistency with similar functions like
WithZEVMRevertOptionsandWithEVMRevertOptions.testutil/sample/crypto.go (1)
10-12: LGTM: Clean import organization.The new Bitcoin-related imports are properly organized and necessary for the added functionality.
zetaclient/chains/bitcoin/observer/witness.go (1)
Line range hint
133-142: Verify consistent memo handling across the codebase.Let's ensure that the memo extraction logic is consistently implemented across all Bitcoin-related files.
✅ Verification successful
Based on the verification results, I can now generate the final response as the scripts have provided comprehensive information about memo handling across the codebase.
Memo handling implementation is consistent and well-structured across Bitcoin-related files
The verification confirms that:
- The
DecodeOpReturnMemofunction signature is consistently used across all implementations- Error handling patterns are uniform across the codebase
- The memo extraction logic in
tryExtractOpRetfollows the established pattern- Test coverage exists for both successful and error cases
The rationale for this verification:
- No instances of old/deprecated
DecodeOpReturnMemosignatures were found- The implementation in
tx_script.goserves as the single source of truth for memo decoding- All usages (
witness.go,inbound.go) follow the same pattern of handling the (memo, found, error) tuple- Comprehensive test coverage in
tx_script_test.govalidates both success and error scenarios🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent memo handling implementation # Expected results: All Bitcoin-related files should use the updated DecodeOpReturnMemo signature # Check for any remaining uses of old DecodeOpReturnMemo signature echo "Checking for old DecodeOpReturnMemo usage patterns..." rg -p "DecodeOpReturnMemo.*tx.*Txid" --type go # Check for consistent memo extraction patterns echo "Checking for memo extraction patterns..." rg -p "tryExtractOpRet|DecodeOpReturnMemo" --type go -A 3Length of output: 2727
pkg/memo/fields_v0.go (2)
Line range hint
66-91: Validation logic is robust and well-structured.The validation method effectively enforces:
- Address validation for both receiver and abort addresses
- Payload restrictions for deposit operations
- Semantic correctness of revert message usage
57-63: Document validation requirements after Unpack signature change.The removal of validation from
Unpackrequires careful handling to ensure validation is performed at the appropriate point in the call chain. Consider adding documentation to clarify when validation should be performed.Let's verify all callers properly handle validation:
✅ Verification successful
Let me search for more specific information about the validation flow.
Validation is properly handled after Unpack signature change
The code analysis reveals that validation is still properly enforced in the call chain:
Unpackis called inmemo.go:DecodeFromBytes()- Validation is explicitly performed right after unpacking:
err = memo.FieldsV0.Validate(memo.OpCode, memo.Header.DataFlags)The separation of validation from
Unpackfollows the single responsibility principle while maintaining security through explicit validation calls. The change improves code organization without compromising validation requirements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Unpack calls to ensure they're paired with Validate rg -A 5 "Unpack\("Length of output: 90098
Script:
#!/bin/bash # Search for Validate calls and memo package usage rg -A 5 "Validate\(" pkg/memo/ rg -A 5 "FieldsV0" pkg/memo/Length of output: 15988
pkg/memo/memo_test.go (2)
255-256: LGTM! Clear and specific error message.The error message clearly indicates the specific failure point in unpacking FieldsV0, which helps in debugging and understanding test failures.
280-286: LGTM! Well-structured error handling.The error handling logic clearly distinguishes between validation failures and decoding failures, with appropriate assertions for each case. The comments effectively explain the expected behavior.
pkg/memo/fields_v0_test.go (1)
216-216: Verify test coverage for operation-specific field validation.While removing
opCodefromUnpacksimplifies the API, we should ensure that operation-specific field validation is still covered elsewhere in the test suite. This is particularly important given the PR's focus on standardizing Bitcoin inbound data and handling different operation types (deposit, depositAndCall).Let's verify the test coverage:
✅ Verification successful
Let me gather more information about the field validation and operation code handling to make a conclusive verification.
Let me gather one final piece of information to understand how operation validation is handled in the new structure.
Operation-specific validation remains robust despite API change
The removal of
opCodefrom theUnpackmethod is a valid architectural change as the verification shows:
- Operation-specific validation is properly handled in the separate
Validatemethod, which still takesopCodeas a parameter- The
Validatemethod contains explicit checks for different operations (e.g., preventing payload for deposit operations)- Test coverage for field validation is maintained through comprehensive test cases in
Test_V0_Validate- The separation of unpacking and validation concerns actually improves the code design by making the validation rules more explicit and centralized
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if operation-specific field validation is covered in other test files # Look for test functions that validate fields based on operation type # Search for test functions that handle operation validation rg -g '*.go' -A 5 'func.*Test.*Operation|func.*Test.*OpCode' # Search for field validation with different operation codes ast-grep --pattern 'fields.Validate($opCode, $_)'Length of output: 1215
Script:
#!/bin/bash # Search for field validation tests and operation code handling # Look for test cases and validation logic in the memo package # Check for field validation tests rg -g 'pkg/memo/*.go' -B 2 -A 5 'func.*Test.*Fields|func.*Test.*Validate' # Look for operation code related constants and types rg -g 'pkg/memo/*.go' "type.*Op|const.*Op" # Search for validation logic in the fields implementation ast-grep --pattern 'func ($receiver) Validate($$$) { $$$ }'Length of output: 2035
Script:
#!/bin/bash # Check the implementation of field validation and operation handling # Look for the Unpack implementation and its validation logic ast-grep --pattern 'func ($receiver) Unpack($$$) error { $$$ }' # Check for any operation-specific validation in the V0 fields rg -g 'pkg/memo/fields_v0.go' -B 2 -A 10 'func.*Validate|func.*Unpack'Length of output: 1398
cmd/zetae2e/local/local.go (1)
303-308: LGTM: Well-structured test coverage for Bitcoin standard memo functionality.The new test cases provide comprehensive coverage for the standard memo functionality, including basic deposits, deposit-and-call operations, and revert scenarios. The test names are clear and follow the established naming convention.
zetaclient/chains/bitcoin/tx_script_test.go (3)
343-349: LGTM! Test case properly updated for the new function signature.The test case correctly validates the OP_RETURN memo decoding functionality with the updated function signature.
353-358: LGTM! Non-OP_RETURN test case properly handles the signature change.The test case maintains good error handling and validation logic while adapting to the new function signature.
359-367: LGTM! Good addition of edge case test.The new test case for short scripts enhances the test coverage by validating an important edge case.
e2e/e2etests/e2etests.go (1)
498-541: Verify implementation of referenced test functions.Let's ensure that all the new test functions referenced in the registrations are implemented in the codebase.
✅ Verification successful
Let me try a different search strategy using
rgto locate these function implementations, as they might be in different files or have different patterns.
All test functions are properly implemented
The verification confirms that all test functions referenced in the registrations are implemented in their respective files:
TestBitcoinDepositAndCallRevertintest_bitcoin_deposit_and_call_revert.goTestBitcoinDonationintest_bitcoin_donation.goTestBitcoinStdMemoDepositintest_bitcoin_std_deposit.goTestBitcoinStdMemoDepositAndCallintest_bitcoin_std_deposit_and_call.goTestBitcoinStdMemoDepositAndCallRevertintest_bitcoin_std_deposit_and_call_revert.goTestBitcoinStdMemoDepositAndCallRevertOtherAddressintest_bitcoin_std_deposit_and_call_revert_other_address.go🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all referenced test functions are implemented. # Test functions to verify functions=( "TestBitcoinDepositAndCallRevert" "TestBitcoinDonation" "TestBitcoinStdMemoDeposit" "TestBitcoinStdMemoDepositAndCall" "TestBitcoinStdMemoDepositAndCallRevert" "TestBitcoinStdMemoDepositAndCallRevertOtherAddress" ) # Search for function implementations for func in "${functions[@]}"; do echo "Searching for function: $func" ast-grep --pattern "func $func($$$) $$$" doneLength of output: 1621
Script:
#!/bin/bash # Search for function implementations using ripgrep # Using -A 1 to show one line after the match for better context for func in "TestBitcoinDepositAndCallRevert" "TestBitcoinDonation" "TestBitcoinStdMemoDeposit" "TestBitcoinStdMemoDepositAndCall" "TestBitcoinStdMemoDepositAndCallRevert" "TestBitcoinStdMemoDepositAndCallRevertOtherAddress"; do echo "=== Searching for function: $func ===" rg "func $func" -A 1 done # Also search for the function names in case they're referenced differently echo "=== Searching for any references to these functions ===" rg "TestBitcoin(Deposit|Donation|StdMemo)" --type goLength of output: 8728
zetaclient/chains/bitcoin/observer/event.go (4)
38-63: StructBTCInboundEventis well-definedThe
BTCInboundEventstruct encapsulates all necessary fields for representing an inbound Bitcoin transaction event. The use of fields likeFromAddress,ToAddress,Value, andMemoBytesprovides clarity and completeness.
66-89:CheckProcessabilityfunction logic is soundThe method
CheckProcessabilitysystematically checks for compliance violations, donation identification, and correctly categorizes the inbound event's processability. The logic is clear and adheres to the compliance requirements.
145-147: Validate the receiver address before assignmentThe check for an empty receiver address is appropriate. Ensuring that
receiveris not empty before assigning toevent.ToAddressprevents potential issues downstream.
200-233: 🛠️ Refactor suggestionConsider updating the protocol contract version
The function
NewInboundVoteMemoStdis currently creating a message withProtocolContractVersion_V1, even though it's intended for standard memos, which might be associated with a newer protocol version.Update the
ProtocolContractVersionto reflect the appropriate version:crosschaintypes.NewMsgVoteInbound( // Existing parameters - crosschaintypes.ProtocolContractVersion_V1, + crosschaintypes.ProtocolContractVersion_V2, false, // Update if relevant for v2 )Also, consider updating the TODO comment and renaming the function to
EventToInboundVoteV2when addressing Issue #2711.Likely invalid or redundant comment.
e2e/utils/zetacore.go (1)
209-212: 🛠️ Refactor suggestionEnsure Consistent Parameter Ordering
The parameter
t require.TestingTis placed after the context inWaitCctxRevertedByInboundHash. For consistency and readability, consider placing the testing parameter after the context to align with common Go conventions and the rest of the codebase.You may adjust the function signature as follows:
func WaitCctxRevertedByInboundHash( ctx context.Context, + t require.TestingT, hash string, c CCTXClient, - t require.TestingT, ) crosschaintypes.CrossChainTx {Likely invalid or redundant comment.
zetaclient/chains/bitcoin/tx_script.go (2)
178-180: Confirm the handling of non-OP_RETURN scripts.The function returns
nil, false, nilwhen the script is not an OP_RETURN script. Please ensure that this behavior aligns with the expectations of the callers and that they correctly handle the absence of memo data without misinterpreting it as an error.
169-211: TheDecodeOpReturnMemofunction demonstrates robust error handling and clear logic.The implementation correctly decodes OP_RETURN scripts, accounts for different data length representations, and provides informative error messages, enhancing maintainability and readability.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 204-204: zetaclient/chains/bitcoin/tx_script.go#L204
Added line #L204 was not covered by testse2e/runner/bitcoin.go (5)
24-24: Import ofmemopackage is appropriateThe addition of the
"github.com/zeta-chain/node/pkg/memo"import is necessary for handling memo functionality in the updated methods.
Line range hint
81-111: Modification ofDepositBTCWithAmountmethod supports memosThe updated signature of
DepositBTCWithAmount, accepting a*memo.InboundMemo, effectively enables handling both standard and legacy memos based on the memo parameter. The conditional logic is clear and correctly directs the flow to the appropriate deposit method.
178-184: Legacy memo handling inDepositBTCWithLegacyMemois accurateThe
DepositBTCWithLegacyMemofunction correctly uses the deployer's EVM address bytes as the memo for legacy deposits, aligning with the expected behavior for legacy memo processing.
188-201: Standard memo implementation inDepositBTCWithStandardMemois correctThe function
DepositBTCWithStandardMemoproperly encodes theInboundMemoto bytes and utilizes it in the deposit process. Error handling is appropriate withrequire.NoError(r, err)ensuring any encoding issues are caught.
405-410: Ensure correct parsing of the receiver address from transaction outputThe method accurately extracts the second output (
TxOut[1]) to retrieve the receiver's address, adhering to the expected transaction structure. Error handling after decoding the script is correctly implemented.
|
Tested by disabling the mainnet condition (like mainnet is used for E2E tests), this looks good |
…ard memo is disabled for mainnet
|
| @@ -22,6 +22,13 @@ const MaxMessageLength = 10240 | |||
| // InboundVoteOption is a function that sets some option on the inbound vote message | |||
| type InboundVoteOption func(*MsgVoteInbound) | |||
|
|
|||
| // WithMemoRevertOptions sets the revert options for inbound vote message | |||
There was a problem hiding this comment.
The function comment referring to the WithMemoRevertOptions function is outdated, the function is called WithRevertOptions.
Description
Complete:
deposit,deposit_and_call,deposit_and_call_revert, etc.Closes: #2880
Closes: #2149
How Has This Been Tested?
Summary by CodeRabbit
New Features
zetacoredRPC package.Bug Fixes
Tests
Documentation