Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request integrates an “execute revert” functionality into Solana operations. It adds new changelog entries, test cases, and constants to validate deposit, call, and withdrawal scenarios involving revert behavior. Several structs and methods across modules have been updated to support revert-related data and control flow, including changes in instruction discriminators, message construction, and signature handling. Additionally, dependency versions and compute unit limits have been updated, and new JSON test data reflecting the transaction metadata has been introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant S as Signer
participant G as Gateway
participant B as Blockchain
C->>S: Submit transaction request (with revert flag)
S->>S: Evaluate transaction type (Normal vs Revert)
alt Revert Execution
S->>S: Call createAndSignMsgExecute(revert=true)
S->>G: Build executeRevert instruction
else Normal Execution
S->>S: Call createAndSignMsgExecute(revert=false)
S->>G: Build standard execute instruction
end
G->>B: Submit instruction to blockchain
B-->>G: Return transaction result
G-->>S: Relay transaction status and messages
S-->>C: Return final status (including revert details if applicable)
Possibly related PRs
Suggested labels
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
|
skosito
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3790 +/- ##
===========================================
- Coverage 64.28% 64.20% -0.08%
===========================================
Files 463 463
Lines 33271 33308 +37
===========================================
- Hits 21387 21386 -1
- Misses 10900 10936 +36
- Partials 984 986 +2
🚀 New features to boost your workflow:
|
skosito
changed the title
|
!!!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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
e2e/e2etests/test_solana_deposit_and_call_revert_with_call_that_reverts.go (2)
24-27: Same TODO issue as previous file.The TODO comment about repeated reverter contract deployments appears here as well. The same refactoring suggestion applies.
See the refactoring suggestion in test_solana_deposit_and_call_revert_with_call.go for a solution to avoid repeated deployments.
69-78: Duplicate ConnectedPdaInfo definition.The ConnectedPdaInfo struct is defined here exactly as in the other test file. This reinforces the need for a shared definition.
See the refactoring suggestion in test_solana_deposit_and_call_revert_with_call.go for extracting this struct to a shared location.
🧹 Nitpick comments (11)
e2e/runner/solana.go (2)
458-458: Significant increase in compute unit limit for SOL transactions.The compute unit limit for SOL transactions has been increased from 100,000 to 500,000, which is a 5x increase. This change is likely necessary to support the more complex execute_revert functionality, but it may have implications for transaction fees.
Consider documenting the rationale for this increase in a comment to provide context for future developers.
- limit := computebudget.NewSetComputeUnitLimitInstruction(500000).Build() // 500k compute unit limit + limit := computebudget.NewSetComputeUnitLimitInstruction(500000).Build() // 500k compute unit limit - increased from 100k to support execute_revert operations
490-490: Consider updating compute unit limit for SOLCall method.For consistency, you might want to update the compute unit limit in the SOLCall method as well, which still uses 100,000 units. If execute_revert operations require more computational resources, regular call operations might benefit from the increased limit too.
- limit := computebudget.NewSetComputeUnitLimitInstruction(100000).Build() // 100k compute unit limit + limit := computebudget.NewSetComputeUnitLimitInstruction(500000).Build() // 500k compute unit limit - consistent with SOLDepositAndCalle2e/e2etests/test_solana_deposit_and_call_revert_with_call.go (2)
23-26: Address TODO comment about repeated reverter contract deployments.The TODO comment indicates a potential optimization opportunity. Consider refactoring to deploy the reverter contract once and reuse it across tests to improve test execution time.
Extract the reverter contract deployment to a shared setup function that can be called by multiple test functions:
- // TODO: consider removing repeated deployments of reverter contract - reverterAddr, _, _, err := testcontract.DeployReverter(r.ZEVMAuth, r.ZEVMClient) - require.NoError(r, err) - r.Logger.Info("Reverter contract deployed at: %s", reverterAddr.String()) + reverterAddr := deployOrGetReverterContract(r)Add a helper function to deploy or retrieve the contract:
// deployOrGetReverterContract deploys the reverter contract once or returns the existing address func deployOrGetReverterContract(r *runner.E2ERunner) common.Address { // Use a package-level variable to cache the deployment if reverterContractAddr == (common.Address{}) { addr, _, _, err := testcontract.DeployReverter(r.ZEVMAuth, r.ZEVMClient) require.NoError(r, err) r.Logger.Info("Reverter contract deployed at: %s", addr.String()) reverterContractAddr = addr } return reverterContractAddr }
68-78: Consider making ConnectedPdaInfo reusable.This struct is defined multiple times across different test files. Consider extracting it to a shared package to improve maintainability.
Move the ConnectedPdaInfo struct to a shared location:
- type ConnectedPdaInfo struct { - Discriminator [8]byte - LastSender [20]byte - LastMessage string - LastRevertSender solana.PublicKey - LastRevertMessage string - } - pda := ConnectedPdaInfo{} + pda := utils.ConnectedPdaInfo{}And define it once in a shared file like
e2e/utils/types.go:// ConnectedPdaInfo represents the structure of data stored in the connected PDA type ConnectedPdaInfo struct { Discriminator [8]byte LastSender [20]byte LastMessage string LastRevertSender solana.PublicKey LastRevertMessage string }e2e/e2etests/test_solana_deposit_and_call_revert_with_call_that_reverts.go (1)
15-17: Function comment has a typo.There's a typo in the function comment: "revets" should be "reverts".
-// with revert options when call on revert program revets +// with revert options when call on revert program revertszetaclient/chains/solana/signer/execute.go (1)
68-84: New branching forexecute_revertserialization.
This is a clear separation of concerns. Verify that you handle any additional fields needed in a real-world revert scenario (e.g., logs or revert reason data) to ensure robust error reporting.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 68-84: zetaclient/chains/solana/signer/execute.go#L68-L84
Added lines #L68 - L84 were not covered by testspkg/contracts/solana/instruction.go (3)
344-345: Remove duplicate interface assertionThe assertion
var _ OutboundInstruction = (*ExecuteInstructionParams)(nil)already exists on line 273, making this line redundant.-var _ OutboundInstruction = (*ExecuteInstructionParams)(nil)
355-356: Consider consistency in using field type for SenderThe
Senderfield type issolana.PublicKeyhere while the corresponding field inExecuteInstructionParams(line 284) uses[20]byte. For consistency, consider using the same type across both structs unless there's a specific reason for the difference.
397-399: Correct the function commentThe function comment incorrectly refers to 'execute' but this function parses 'execute_revert'.
-// ParseInstructionExecute tries to parse the instruction as a 'execute_revert'. -// It returns nil if the instruction can't be parsed as a 'execute_revert'. +// ParseInstructionExecuteRevert tries to parse the instruction as a 'execute_revert'. +// It returns nil if the instruction can't be parsed as a 'execute_revert'.zetaclient/chains/solana/signer/signer.go (1)
407-417: Clearly separate revert message handling logicThe message handling logic now has two branches (revert and non-revert). Consider extracting this into a helper function to improve readability and maintainability.
- isRevert := cctx.CctxStatus.Status == types.CctxStatus_PendingRevert && cctx.RevertOptions.CallOnRevert - var message []byte - if isRevert { - message = cctx.RevertOptions.RevertMessage - } else { - messageToDecode, err := hex.DecodeString(cctx.RelayedMessage) - if err != nil { - return nil, errors.Wrapf(err, "decodeString %s error", cctx.RelayedMessage) - } - message = messageToDecode - } + isRevert := cctx.CctxStatus.Status == types.CctxStatus_PendingRevert && cctx.RevertOptions.CallOnRevert + message, err := getExecuteMessage(cctx, isRevert) + if err != nil { + return nil, err + } // Add this helper function elsewhere in the file func getExecuteMessage(cctx *types.CrossChainTx, isRevert bool) ([]byte, error) { if isRevert { return cctx.RevertOptions.RevertMessage, nil } messageToDecode, err := hex.DecodeString(cctx.RelayedMessage) if err != nil { return nil, errors.Wrapf(err, "decodeString %s error", cctx.RelayedMessage) } return messageToDecode, nil }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 407-416: zetaclient/chains/solana/signer/signer.go#L407-L416
Added lines #L407 - L416 were not covered by testspkg/contracts/solana/gateway_message.go (1)
314-319: Consider using polymorphism instead of conditional logicThe
Hash()method now includes conditional logic to determine which instruction byte to use. For better maintainability and extensibility, consider using polymorphism by creating separate message types for execute and execute_revert.This would involve creating a base message type and two derived types, each with their own
Hash()implementation. However, since this would require more extensive refactoring, the current approach is acceptable for now.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
contrib/localnet/solana/connected.sois excluded by!**/*.socontrib/localnet/solana/gateway.sois excluded by!**/*.sogo.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
changelog.md(1 hunks)cmd/zetae2e/local/local.go(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_solana_deposit_and_call_revert_with_call.go(1 hunks)e2e/e2etests/test_solana_deposit_and_call_revert_with_call_that_reverts.go(1 hunks)e2e/e2etests/test_solana_withdraw_and_call.go(1 hunks)e2e/e2etests/test_spl_withdraw_and_call.go(1 hunks)e2e/runner/solana.go(1 hunks)go.mod(1 hunks)pkg/contracts/solana/gateway.go(1 hunks)pkg/contracts/solana/gateway.json(1 hunks)pkg/contracts/solana/gateway_message.go(6 hunks)pkg/contracts/solana/gateway_message_test.go(2 hunks)pkg/contracts/solana/instruction.go(1 hunks)zetaclient/chains/solana/observer/outbound.go(1 hunks)zetaclient/chains/solana/observer/outbound_test.go(2 hunks)zetaclient/chains/solana/signer/execute.go(4 hunks)zetaclient/chains/solana/signer/signer.go(3 hunks)zetaclient/testdata/solana/chain_901_outbound_tx_result_4QjCYR4CfS5RFUQuRS8W68ZpBgqd91zZmC5Z1M4uyh4BeZWnB6NtRMxwwZttyre344zX6vTme2Eum94BHQ5Xk9Tf.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/observer/outbound.gocmd/zetae2e/local/local.goe2e/runner/solana.gopkg/contracts/solana/gateway.gozetaclient/chains/solana/observer/outbound_test.goe2e/e2etests/test_solana_withdraw_and_call.goe2e/e2etests/test_spl_withdraw_and_call.goe2e/e2etests/test_solana_deposit_and_call_revert_with_call.goe2e/e2etests/test_solana_deposit_and_call_revert_with_call_that_reverts.goe2e/e2etests/e2etests.gozetaclient/chains/solana/signer/execute.gopkg/contracts/solana/instruction.gopkg/contracts/solana/gateway_message_test.gopkg/contracts/solana/gateway_message.gozetaclient/chains/solana/signer/signer.go
🧬 Code Graph Analysis (8)
zetaclient/chains/solana/observer/outbound.go (1)
pkg/contracts/solana/instruction.go (3)
ParseInstructionWithdraw(257-271)ParseInstructionExecute(328-342)ParseInstructionExecuteRevert(399-413)
cmd/zetae2e/local/local.go (1)
e2e/e2etests/e2etests.go (2)
TestSolanaDepositAndCallRevertWithCallName(69-69)TestSolanaDepositAndCallRevertWithCallThatRevertsName(70-70)
zetaclient/chains/solana/observer/outbound_test.go (2)
zetaclient/chains/solana/rpc/rpc.go (1)
GetTransaction(130-148)pkg/contracts/solana/instruction.go (1)
ParseInstructionExecuteRevert(399-413)
e2e/e2etests/test_solana_deposit_and_call_revert_with_call.go (8)
e2e/utils/parsing.go (1)
ParseBigInt(27-32)pkg/contracts/solana/gateway.go (2)
ComputeConnectedPdaAddress(92-94)ComputePdaAddress(82-89)e2e/runner/solana.go (1)
ConnectedProgramID(24-24)pkg/contracts/solana/instruction.go (2)
GetExecuteMsgAbi(640-662)ExecuteMsg(634-637)e2e/utils/contracts.go (1)
ErrHashRevertFoo(14-14)e2e/e2etests/test_solana_deposit_and_call_revert_with_call_that_reverts.go (1)
ConnectedPdaInfo(69-75)e2e/e2etests/test_spl_withdraw_and_call.go (1)
ConnectedPdaInfo(92-98)e2e/e2etests/test_solana_withdraw_and_call.go (1)
ConnectedPdaInfo(89-95)
e2e/e2etests/test_solana_deposit_and_call_revert_with_call_that_reverts.go (5)
e2e/runner/runner.go (1)
E2ERunner(80-204)e2e/utils/parsing.go (1)
ParseBigInt(27-32)e2e/runner/solana.go (1)
ConnectedProgramID(24-24)pkg/contracts/solana/instruction.go (2)
GetExecuteMsgAbi(640-662)ExecuteMsg(634-637)e2e/utils/contracts.go (1)
ErrHashRevertFoo(14-14)
e2e/e2etests/e2etests.go (3)
e2e/runner/e2etest.go (3)
NewE2ETest(30-47)ArgDefinition(50-53)WithMinimumVersion(13-17)e2e/e2etests/test_solana_deposit_and_call_revert_with_call.go (1)
TestSolanaDepositAndCallRevertWithCall(16-84)e2e/e2etests/test_solana_deposit_and_call_revert_with_call_that_reverts.go (1)
TestSolanaDepositAndCallRevertWithCallThatReverts(17-81)
zetaclient/chains/solana/signer/execute.go (2)
pkg/contracts/solana/instruction.go (3)
AccountMeta(628-631)ExecuteRevertInstructionParams(347-371)ExecuteInstructionParams(276-300)pkg/contracts/solana/gateway_message.go (1)
NewMsgExecute(248-266)
pkg/contracts/solana/instruction.go (3)
zetaclient/chains/solana/signer/signer.go (1)
Signer(54-69)zetaclient/chains/base/signer.go (1)
Signer(18-36)pkg/contracts/solana/gateway.go (1)
DiscriminatorExecuteRevert(49-49)
🪛 GitHub Check: codecov/patch
zetaclient/chains/solana/observer/outbound.go
[warning] 344-345: zetaclient/chains/solana/observer/outbound.go#L344-L345
Added lines #L344 - L345 were not covered by tests
[warning] 347-348: zetaclient/chains/solana/observer/outbound.go#L347-L348
Added lines #L347 - L348 were not covered by tests
zetaclient/chains/solana/signer/execute.go
[warning] 46-46: zetaclient/chains/solana/signer/execute.go#L46
Added line #L46 was not covered by tests
[warning] 68-84: zetaclient/chains/solana/signer/execute.go#L68-L84
Added lines #L68 - L84 were not covered by tests
[warning] 86-100: zetaclient/chains/solana/signer/execute.go#L86-L100
Added lines #L86 - L100 were not covered by tests
[warning] 102-102: zetaclient/chains/solana/signer/execute.go#L102
Added line #L102 was not covered by tests
zetaclient/chains/solana/signer/signer.go
[warning] 165-166: zetaclient/chains/solana/signer/signer.go#L165-L166
Added lines #L165 - L166 were not covered by tests
[warning] 407-416: zetaclient/chains/solana/signer/signer.go#L407-L416
Added lines #L407 - L416 were not covered by tests
[warning] 436-436: zetaclient/chains/solana/signer/signer.go#L436
Added line #L436 was not covered by tests
[warning] 439-439: zetaclient/chains/solana/signer/signer.go#L439
Added line #L439 was not covered by tests
🔇 Additional comments (37)
changelog.md (1)
20-20: LGTM: Clean changelog entry added for PR #3790.The changelog entry follows the established pattern and clearly indicates the purpose of this pull request.
pkg/contracts/solana/gateway.go (1)
48-49: LGTM: Added discriminator constant for new execute_revert instruction.The addition follows the established pattern for instruction discriminators and is properly documented with a clear comment.
go.mod (1)
315-315:Details
✅ Verification successful
Verify updated dependency compatibility.
The protocol-contracts-solana/go-idl dependency has been updated to a newer version (20250408183930 from 20250320221859). This update is likely necessary to support the new execute_revert functionality.
🏁 Script executed:
#!/bin/bash # Verify the compatibility of the updated dependency with the codebase # Check if the updated dependency introduces any breaking changes go list -m -json github.com/zeta-chain/protocol-contracts-solana/go-idl@v0.0.0-20250408183930-aee4e8908b9aLength of output: 735
Dependency Compatibility Verified – No Issues Found
The metadata for the updated dependency:
github.com/zeta-chain/protocol-contracts-solana/go-idl v0.0.0-20250408183930-aee4e8908b9ahas been verified using the
go list -m -jsoncommand. The results show the updated module with the expected commit hash, Go version, and checksum values. This update appears to be correctly aligned with the newexecute_revertfunctionality, with no evidence of breaking changes detected.cmd/zetae2e/local/local.go (1)
444-445: Good addition of test cases to validate the new functionality.The introduction of these two test cases for Solana deposit operations with revert functionality is appropriate. They complement the existing tests and follow the established naming conventions.
zetaclient/chains/solana/observer/outbound.go (1)
340-349:Details
✅ Verification successful
Streamlined instruction parsing with improved control flow.
The refactored code provides a more maintainable and sequential approach to instruction parsing, appropriately adding support for the new ExecuteRevert instruction type.
However, the static analysis indicates that some of this code lacks test coverage. Consider adding tests that would exercise these new code paths:
🏁 Script executed:
#!/bin/bash # Check for existing test coverage of ExecuteRevert parsing rg -A 10 "Test.*ParseInstructionExecuteRevert" --type goLength of output: 996
Test coverage for ExecuteRevert parsing confirmed
The refactored code for instruction parsing is streamlined and maintainable, with the new ExecuteRevert path adequately covered. The test file
zetaclient/chains/solana/observer/outbound_test.goalready includesTest_ParseInstructionExecuteRevert, verifying that the new code path is exercised.No additional test cases are required at this time. Please ensure to keep the tests updated in case of further modifications.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 344-345: zetaclient/chains/solana/observer/outbound.go#L344-L345
Added lines #L344 - L345 were not covered by tests
[warning] 347-348: zetaclient/chains/solana/observer/outbound.go#L347-L348
Added lines #L347 - L348 were not covered by testse2e/e2etests/test_solana_withdraw_and_call.go (1)
93-94: Appropriate extension of the data structure for revert operations.The addition of
LastRevertSenderandLastRevertMessagefields to theConnectedPdaInfostruct properly accommodates the new revert functionality. These fields provide essential context for tracking revert operations in tests.e2e/e2etests/test_spl_withdraw_and_call.go (1)
96-97: Consistent structure extension across related files.The addition of revert-related fields here maintains consistency with the changes in the
test_solana_withdraw_and_call.gofile, ensuring uniform handling of revert operations across different token types.zetaclient/chains/solana/observer/outbound_test.go (4)
55-57: New constant for execute_revert testing looks good.The addition of
executeRevertTxTestfollows the established pattern of defining transaction test constants used throughout the test file.
765-794: LGTM: Test structure follows established pattern.The happy path test case for
ParseInstructionExecuteRevertcorrectly validates the expected behavior by verifying the sender address, gateway nonce, and token amount after parsing.
796-813: Error handling test is implemented appropriately.The test case properly checks the error handling when invalid instruction data is provided, following the pattern established in other similar test functions.
815-835: Discriminator mismatch test is well implemented.The test validates the error case when the instruction's discriminator doesn't match the expected value for an execute_revert instruction.
pkg/contracts/solana/gateway.json (4)
656-669: Clear documentation for the execute_revert instruction.The documentation appropriately describes the purpose of the instruction and its arguments, maintaining consistency with other instruction documentation in the file.
670-679: Unique discriminator for execute_revert instruction.The instruction has a properly defined unique discriminator that differentiates it from other instructions, which is essential for correct instruction parsing.
680-740: Account specification matches execute instruction.The account specifications for
execute_revertfollow the same structure as theexecuteinstruction, which is appropriate given their similar functionality.
655-781:Details
❓ Verification inconclusive
Type inconsistency in sender parameter between execute and execute_revert instructions.
The
senderparameter in theexecute_revertinstruction (line 748) uses apubkeytype, while theexecuteinstruction (line 615) uses an array of bytes"type": {"array": ["u8", 20]}. Verify this inconsistency is intentional and doesn't cause compatibility issues.
🏁 Script executed:
#!/bin/bash # Check how the sender type is used in execute_revert implementation rg -A 5 -B 5 "execute_revert.*sender" --glob "**/*.go" # Check if there are any existing parameters using both types rg --json "sender.*pubkey|sender.*\[u8, 20\]" --glob "**/*.go" | jq '.data.lines.text'Length of output: 330
Action: Verify the intended type conversion for the sender parameter across instructions.
The
senderparameter in theexecute_revertinstruction is defined as apubkey(line 748) while in theexecuteinstruction it is defined as an array of bytes of length 20 (line 615). Preliminary searches did not reveal explicit conversion logic between the two formats in the Go codebase. Please ensure that this discrepancy is intentional and that the conversion (if any) is handled correctly to prevent compatibility issues. If this dual handling is by design (for example, to support legacy interfaces alongside modern representations), consider adding inline comments to clarify the rationale for future maintainers.e2e/e2etests/test_solana_deposit_and_call_revert_with_call.go (4)
38-51: Good use of ABI encoding for message preparation.The code correctly uses the ABI encoding functions to prepare the execute message.
52-57: Properly configured revert options for testing.The test correctly sets up the revert options with the appropriate address, enabling the on_revert call, and providing the encoded message.
59-64: Comprehensive CCTX verification.The test appropriately verifies that the cross-chain transaction is marked as reverted and contains the expected error message.
79-83: Appropriate verification of state changes.The test correctly verifies that the last revert message and sender match the expected values, and that the PDA's balance has increased.
e2e/e2etests/test_solana_deposit_and_call_revert_with_call_that_reverts.go (3)
38-51: Message encoding for revert scenario.The code correctly prepares the message for testing the revert scenario, using the same approach as the previous test file.
59-64: Verify transaction is aborted instead of reverted.This test properly expects an aborted status (different from the previous test which expected a reverted status), highlighting the different testing scenarios.
80-80: Proper verification of unchanged balance.The test correctly verifies that the PDA's balance remains unchanged in the abort scenario, which is the expected behavior.
pkg/contracts/solana/gateway_message_test.go (2)
116-116: Confirm normal execution test coverage.
Your usage of therevertparameter set tofalsehere is appropriate for simulating a standard execution scenario. Please ensure you have corresponding test cases covering both revert and non-revert flows to maintain high coverage.
150-171: Good addition of theTest_MsgExecuteRevertHashtest.
This dedicated test for verifying the new revert-based execution path enhances confidence in your implementation. Confirm that all edge cases (e.g., zero amounts, unexpected metadata) are tested, so that the revert logic is thoroughly validated.zetaclient/testdata/solana/chain_901_outbound_tx_result_4QjCYR4CfS5RFUQuRS8W68ZpBgqd91zZmC5Z1M4uyh4BeZWnB6NtRMxwwZttyre344zX6vTme2Eum94BHQ5Xk9Tf.json (1)
1-114: New JSON fixture for revert scenario looks well-structured.
This file appears to contain no sensitive data and provides detailed logs for debugging. Ensure it remains up to date with any contract log format changes.e2e/e2etests/e2etests.go (2)
63-81: Comprehensive list of new Solana test constants.
Adding clearly named constants improves maintainability and discoverability of your tests. Good job adhering to naming conventions.
648-665: Inclusion of new revert-with-call tests.
These entries successfully expand coverage of Solana deposit and revert scenarios. Verify that any newly introduced dependencies or minimum version logic is satisfied in your CI environment.zetaclient/chains/solana/signer/execute.go (4)
7-7: New import for Ethereum address handling.
The addition ofgithub.com/ethereum/go-ethereum/commonis appropriate for parsing and validating addresses from a string. No concerns here.
46-46: Instantiation ofNewMsgExecutewith revert logic.
Be sure to handle edge cases, such as whensendermight be empty or invalid. To confirm correctness, consider adding further code-level or test-level checks preventing invalid addresses.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 46-46: zetaclient/chains/solana/signer/execute.go#L46
Added line #L46 was not covered by tests
86-100: Fallback to standardExecuteInstructionParams.
The non-revert block is consistent; it parallels the new structure, preserving backward compatibility. Code readability remains good.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 86-100: zetaclient/chains/solana/signer/execute.go#L86-L100
Added lines #L86 - L100 were not covered by tests
102-102: Data bytes assignment for the instruction.
Unit tests exercising both revert and non-revert branches are crucial. Guard against regressions by confirming coverage of this assignment logic in your test suite.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 102-102: zetaclient/chains/solana/signer/execute.go#L102
Added line #L102 was not covered by testspkg/contracts/solana/instruction.go (2)
346-371: LGTM: New struct defined for execute_revert operationThe
ExecuteRevertInstructionParamsstruct is well-defined with appropriate fields and documentation. The struct properly follows the pattern established for other instruction parameter types.
400-412: LGTM: Parsing function is correctly implementedThe
ParseInstructionExecuteRevertfunction correctly deserializes the instruction data and verifies the discriminator.pkg/contracts/solana/gateway_message.go (4)
20-20: LGTM: New instruction identifier addedThe addition of
InstructionExecuteRevertwith value 8 follows the existing pattern for instruction identifiers.
240-242: LGTM: New revert field for distinguishing execution typesAdding the
revertboolean field is a clear way to distinguish between normal execution and revert execution.
304-306: LGTM: Added accessor method for new fieldThe
Revert()method properly exposes the new field, following the pattern established for other fields.
235-235:Details
❓ Verification inconclusive
Verify impacts of changing sender field type
The type of the
senderfield has been changed from[20]bytetostring. Ensure this change doesn't impact other parts of the codebase that might be relying on the original type.
🏁 Script executed:
#!/bin/bash # Check for usages of MsgExecute.sender to identify potential impacts echo "Checking for direct references to MsgExecute.sender field..." rg "\.sender" --type go | grep -i "msgexecute" echo "Checking for calls to Sender() method..." rg "\.Sender\(\)" --type go | grep -i "msgexecute"Length of output: 319
Action Required: Verify Field Type Change Impact for MsgExecute.sender
Based on the automated search, there are no direct references to either the
MsgExecute.senderfield or calls to theSender()method, which suggests that changing its type from[20]bytetostringmay have minimal impact. However, given the limited output from the search, please manually verify that no dependent code relies on the original[20]bytetype.
- Confirm that no modules or interfaces expect a
[20]byteforsender.- Ensure that any indirect or implicit usages in the codebase (e.g., serialization or interfacing code) are updated accordingly.
|
Should the PR in the Solana contract repo be put ready for review? |
e2e/e2etests/test_solana_deposit_and_call_revert_with_call_that_reverts.go
Outdated
Show resolved
Hide resolved
i updated solana PR in PR description here |
3 participants
Description
tried to reuse as much as possible from execute, since these 2 instructions are almost identical, and adding new instruction adds a lot of boilerplate, but let me know if this is not super readable
solana PR zeta-chain/protocol-contracts-solana#101
How Has This Been Tested?
Summary by CodeRabbit