fix: add sender to execute message hash#3896
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 update introduces conditional validation and formatting of sender addresses based on the execution type for Solana gateway messages. The sender is now handled as either an Ethereum or Solana address depending on context, with explicit validation and normalization. Hash calculation logic and corresponding tests are updated to reflect these changes, and error handling is improved to prevent panics during public key parsing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Signer
participant Validator
Client->>Signer: Request to create MsgExecute/MsgExecuteSPL
Signer->>Signer: validateSender(sender, executeType)
alt executeType == Call
Signer->>Signer: Validate as Ethereum address
else executeType == Revert
Signer->>Signer: Validate as Solana public key
end
Signer->>Signer: Construct message with validated sender
Signer->>Validator: Send message for processing
Validator-->>Signer: Validation/Execution result
Signer-->>Client: Response
sequenceDiagram
participant System
participant decodeMintAccountDetails
System->>decodeMintAccountDetails: Provide asset (public key string)
decodeMintAccountDetails->>decodeMintAccountDetails: Parse asset with error handling
alt Invalid public key
decodeMintAccountDetails-->>System: Return error
else Valid public key
decodeMintAccountDetails->>decodeMintAccountDetails: Query account info
decodeMintAccountDetails-->>System: Return account details
end
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3896 +/- ##
===========================================
- Coverage 64.99% 64.94% -0.05%
===========================================
Files 470 470
Lines 34354 34379 +25
===========================================
Hits 22329 22329
- Misses 10993 11018 +25
Partials 1032 1032
🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
zetaclient/chains/solana/signer/withdraw_spl.go (1)
160-165: Excellent defensive programming improvement.The change from
MustPublicKeyFromBase58toPublicKeyFromBase58with proper error handling prevents potential panics and aligns with robust error handling practices. This ensures the application can gracefully handle invalid public key strings rather than crashing.Consider adding unit tests to cover the error path when
PublicKeyFromBase58fails:func TestSigner_decodeMintAccountDetails_InvalidAsset(t *testing.T) { signer := &Signer{} // initialize appropriately _, err := signer.decodeMintAccountDetails(context.Background(), "invalid-base58") require.Error(t, err) require.Contains(t, err.Error(), "invalid character") }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 160-163: zetaclient/chains/solana/signer/withdraw_spl.go#L160-L163
Added lines #L160 - L163 were not covered by tests
[warning] 165-165: zetaclient/chains/solana/signer/withdraw_spl.go#L165
Added line #L165 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contrib/localnet/solana/gateway.sois excluded by!**/*.so
📒 Files selected for processing (6)
changelog.md(1 hunks)pkg/contracts/solana/gateway_message.go(2 hunks)pkg/contracts/solana/gateway_message_test.go(4 hunks)zetaclient/chains/solana/signer/execute.go(4 hunks)zetaclient/chains/solana/signer/execute_spl.go(2 hunks)zetaclient/chains/solana/signer/withdraw_spl.go(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/signer/withdraw_spl.gozetaclient/chains/solana/signer/execute_spl.gozetaclient/chains/solana/signer/execute.gopkg/contracts/solana/gateway_message_test.gopkg/contracts/solana/gateway_message.go
🪛 GitHub Check: codecov/patch
zetaclient/chains/solana/signer/withdraw_spl.go
[warning] 160-163: zetaclient/chains/solana/signer/withdraw_spl.go#L160-L163
Added lines #L160 - L163 were not covered by tests
[warning] 165-165: zetaclient/chains/solana/signer/withdraw_spl.go#L165
Added line #L165 was not covered by tests
zetaclient/chains/solana/signer/execute_spl.go
[warning] 98-101: zetaclient/chains/solana/signer/execute_spl.go#L98-L101
Added lines #L98 - L101 were not covered by tests
[warning] 142-142: zetaclient/chains/solana/signer/execute_spl.go#L142
Added line #L142 was not covered by tests
zetaclient/chains/solana/signer/execute.go
[warning] 93-96: zetaclient/chains/solana/signer/execute.go#L93-L96
Added lines #L93 - L96 were not covered by tests
[warning] 111-111: zetaclient/chains/solana/signer/execute.go#L111
Added line #L111 was not covered by tests
[warning] 184-191: zetaclient/chains/solana/signer/execute.go#L184-L191
Added lines #L184 - L191 were not covered by tests
[warning] 195-199: zetaclient/chains/solana/signer/execute.go#L195-L199
Added lines #L195 - L199 were not covered by tests
🔇 Additional comments (5)
changelog.md (1)
52-52: LGTM: Proper changelog documentation.The changelog entry accurately documents the fix for adding sender to Solana execute message hash and follows the established format.
zetaclient/chains/solana/signer/execute_spl.go (1)
97-102:Details
❓ Verification inconclusive
Critical sender validation enhancement.
The addition of sender validation before message creation is essential for ensuring address format consistency based on the execute type. This prevents potential issues with incompatible address formats in subsequent processing.
Please verify that the
validateSenderfunction properly handles all execute types and provide comprehensive test coverage:Add unit tests to ensure sender validation works correctly for different execute types:
func TestCreateMsgExecuteSPL_ValidateSender(t *testing.T) { tests := []struct { name string sender string executeType contracts.ExecuteType expectError bool }{ { name: "valid ethereum address for call", sender: "0x42bd6E2ce4CDb2F58Ed0A0E427F011A0645D5E33", executeType: contracts.ExecuteTypeCall, expectError: false, }, { name: "valid solana address for revert", sender: "CVoPuE3EMu6QptGHLx7mDGb2ZgASJRQ5BcTvmhZNJd8A", executeType: contracts.ExecuteTypeRevert, expectError: false, }, // Add more test cases } // Implementation here }
🏁 Script executed:
#!/bin/bash # Verify validateSender function implementation and usage ast-grep --pattern 'func validateSender($_, $_) ($_, error) { $$$ }' # Check for similar validation in execute.go rg -A 10 "validateSender.*executeType"Length of output: 4195
🏁 Script executed:
#!/bin/bash # Locate ExecuteType definition in the contracts package rg "type ExecuteType" -R . # List all ExecuteType constants and their values rg "ExecuteType.*=" -R . # Confirm the two known execute types rg "ExecuteTypeCall" -R . rg "ExecuteTypeRevert" -R .Length of output: 1729
Ensure validateSender Covers All ExecuteType Variants
The current
validateSenderimplementation only checks forExecuteTypeCall(Ethereum) and otherwise treats every other case as a Solana address. If there are additionalcontracts.ExecuteTypeconstants, they’ll be misrouted to the Solana branch, leading to subtle errors. Please manually verify the complete set ofExecuteTypevalues and, if any beyondExecuteTypeCallandExecuteTypeRevertexist, refactorvalidateSenderto use an explicitswitchand return an error for unsupported cases.• validateSender implementation:
zetaclient/chains/solana/signer/execute.go:184–200
• Invocation sites:
- zetaclient/chains/solana/signer/execute.go
- zetaclient/chains/solana/signer/execute_spl.go
Also add unit tests covering every
ExecuteType(valid and invalid formats, plus unsupported enums), for example:func TestValidateSender_AllExecuteTypes(t *testing.T) { cases := []struct { name string sender string executeType contracts.ExecuteType wantErr bool }{ {"ValidCall", "0x...", contracts.ExecuteTypeCall, false}, {"ValidRevert", "So1anaAddr...", contracts.ExecuteTypeRevert, false}, {"UnsupportedType", "any", contracts.ExecuteTypeMint, true}, // … } // … }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 98-101: zetaclient/chains/solana/signer/execute_spl.go#L98-L101
Added lines #L98 - L101 were not covered by testspkg/contracts/solana/gateway_message_test.go (2)
158-158:Details
❓ Verification inconclusive
Sender type consistency for revert operations.
The change from Ethereum addresses to Solana public keys for revert operations and the corresponding string representation method change (
.Hex()to.String()) properly aligns with the conditional address handling based on execute type.Ensure this change maintains compatibility with existing revert operations and that the gateway contract can properly handle both address formats:
Also applies to: 165-165, 183-183, 190-190
🌐 Web query:
What are the implications of changing sender address formats in Solana gateway execute revert operations? How does this affect backward compatibility?💡 Result:
Based on the search results, there isn't specific information about changing sender address formats in Solana gateway execute revert operations. However, I can provide some insights about revert operations, backward compatibility concerns, and transaction handling in Solana that may be relevant to your query.
Revert Operations in Blockchain Environments
Execution reverts are critical mechanisms in blockchain environments that prevent incorrect transactions from completing. While the search results don't specifically address Solana gateway execute revert operations, they do provide information about EVM execution reverted errors, which might have similar principles.
When smart contracts encounter rule violations, execution reverts occur, functioning as safeguards against incorrect transactions[2]. Common causes include business logic violations, insufficient gas, invalid jumps, stack overflow/underflow, and invalid opcodes[2].
Backward Compatibility Considerations
Backward compatibility is a significant concern when implementing new features or changes in blockchain systems:
Syscall Implementation: The search results indicate that "programs using the new syscall could not be used on Solana version which does not implement this feature. Existing programs that do not use this feature are not impacted at all."[6] This suggests that changes to core functionality like syscalls require feature gates to ensure backward compatibility.
Transaction Version Changes: Solana has implemented versioned transactions to allow additional functionality in the runtime. While changes to onchain programs are not required for compatibility, developers must update client-side code to prevent errors due to different transaction versions[7].
Feature Gating: For significant changes, Solana typically uses feature gates to enable new functionality only when the majority of the cluster runs the required version[6].
Transaction Handling and Error Management
If you're dealing with address format changes that might cause reverts, consider these aspects of Solana's transaction handling:
Preflight Checks: By default, Solana performs preflight checks before submitting transactions, verifying signature validity, blockhash recency, and transaction simulation. Skipping these checks can lead to lost transactions[9].
Retry Logic: Solana allows customizing rebroadcast logic for transactions through the
maxRetriesparameter, enabling developers to implement their own retry strategies for failed transactions[9].Re-signing Transactions: When a transaction fails, developers should ensure the initial transaction's blockhash has expired before re-signing to prevent duplicate transactions[9].
Best Practices for Implementation
When implementing changes that might affect address formats or cause reverts:
Testing: Thoroughly test any changes to address formats with both old and new client code to ensure compatibility.
Error Handling: Implement robust error handling that can gracefully manage reverts and provide clear error messages to users.
Documentation: Clearly document any changes to address formats and their potential impact on existing applications.
While the search results don't directly address the implications of changing sender address formats in Solana gateway execute revert operations, these general principles from Solana's development ecosystem should help guide implementation decisions that maintain backward compatibility while enabling new functionality.
Citations:
- 1: https://www.zetachain.com/docs/developers/tutorials/call/
- 2: https://metana.io/blog/evm-execution-reverted-errors/
- 3: https://solana.com/fr/developers/guides/advanced/exchange
- 4: https://github.com/wormhole-foundation/wormhole-sdk-ts
- 5: https://docs.chainstack.com/docs/solana-how-to-priority-fees-faster-transactions
- 6: https://github.com/solana-foundation/solana-improvement-documents/blob/main/proposals/0047-syscall-and-sysvar-for-last-restart-slot.md
- 7: https://solana.com/developers/guides/advanced/versions
- 8: https://docs.tatum.io/docs/token-decimals-erc-20-or-compatible
- 9: https://solana.com/developers/guides/advanced/retry
- 10: https://chainstack.com/solana-how-to-troubleshoot-common-development-errors/
Validate Sender Format Consistency for Revert Operations
The switch from Ethereum-style hex addresses (
.Hex()) to Solana public keys (.String()) in revert tests correctly reflects our multi-chain handling. To ensure backward compatibility and prevent regressions:• Add or update unit tests exercising both legacy Ethereum and new Solana address formats in revert flows.
• Verify the gateway contract’s revert logic accepts and logs.String()-based public keys without breaking existing clients.
• Confirm downstream consumers (logging, monitoring, client SDKs) handle the new format seamlessly.
• Update documentation to clearly state supported sender formats for revert operations.Locations to review:
- pkg/contracts/solana/gateway_message_test.go @ lines 158, 165, 183, 190
111-111:Details
❓ Verification inconclusive
Hash values updated for execute type changes.
The updated expected hash values reflect the new hash calculation logic that conditionally includes sender information based on execute type.
Verify that the new hash values are computed correctly and align with the gateway contract expectations:
Also applies to: 137-137
🏁 Script executed:
#!/bin/bash # Check the hash calculation logic in gateway_message.go ast-grep --pattern 'func ($m *MsgExecute) Hash() [32]byte { $$$ }' ast-grep --pattern 'func ($m *MsgExecuteSPL) Hash() [32]byte { $$$ }'Length of output: 144
To locate the hash‐calculation logic so we can confirm the updated test vectors, let’s dump all
Hash()methods and their surrounding types ingateway_message.go:
🏁 Script executed:
#!/bin/bash # 1. List all Hash() methods returning [32]byte in gateway_message.go rg -n 'Hash() \[32\]byte' --color=never pkg/contracts/solana/gateway_message.go # 2. Show context around those method definitions rg -n 'func.*Hash' --color=never pkg/contracts/solana/gateway_message.go -A5 -B2 # 3. Show MsgExecute type definition rg -n 'type MsgExecute' --color=never pkg/contracts/solana/gateway_message.go -A5 # 4. Show MsgExecuteSPL type definition rg -n 'type MsgExecuteSPL' --color=never pkg/contracts/solana/gateway_message.go -A5Length of output: 2488
To confirm that sender-conditional logic is present, let’s dump the full
Hash()implementations for both execute types:
🏁 Script executed:
#!/bin/bash file="pkg/contracts/solana/gateway_message.go" # MsgExecute.Hash start=$(grep -n 'func (msg \*MsgExecute) Hash() \[32\]byte' "$file" | cut -d: -f1) end_rel=$(tail -n +$start "$file" | grep -n '^}' | head -1 | cut -d: -f1) end=$((start + end_rel - 1)) echo "=== MsgExecute.Hash() ===" sed -n "${start},${end}p" "$file" # MsgExecuteSPL.Hash start2=$(grep -n 'func (msg \*MsgExecuteSPL) Hash() \[32\]byte' "$file" | cut -d: -f1) end_rel2=$(tail -n +$start2 "$file" | grep -n '^}' | head -1 | cut -d: -f1) end2=$((start2 + end_rel2 - 1)) echo "=== MsgExecuteSPL.Hash() ===" sed -n "${start2},${end2}p" "$file"Length of output: 2627
Please verify updated Hash() logic for execute messages
The
Hash()implementations for bothMsgExecute(lines 319–351) andMsgExecuteSPL(lines 632–666) now include conditional branches that choose betweenInstructionExecute/InstructionExecuteRevert(or their SPL counterparts) and encode the sender via eithercommon.HexToAddressorsolana.MustPublicKeyFromBase58based onexecuteType. The test file atpkg/contracts/solana/gateway_message_test.go(lines 111 and 137) was updated with new expected hash strings.• pkg/contracts/solana/gateway_message.go
– MsgExecute.Hash() (lines 319–351)
– MsgExecuteSPL.Hash() (lines 632–666)
• pkg/contracts/solana/gateway_message_test.go
– wantHash at line 111
– wantHash at line 137Run the full test suite or independently compute the keccak256 of the constructed message bytes for both
ExecuteTypeCalland revert flows to confirm those hex strings match the gateway contract’s spec.zetaclient/chains/solana/signer/execute.go (1)
92-96: Sender validation implementation is correct.The addition of sender validation based on execute type is a critical security improvement that ensures address format consistency before message creation.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 93-96: zetaclient/chains/solana/signer/execute.go#L93-L96
Added lines #L93 - L96 were not covered by tests
4 participants
Description
https://github.com/zeta-chain/protocol-private/issues/262
solana PR: zeta-chain/protocol-contracts-solana#111
also adds validation for sender before creation execute msg
How Has This Been Tested?
Summary by CodeRabbit