fix: integrate deposit fee and audit fixes #3232
Conversation
skosito
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie and
swift1337
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📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files related to the Solana gateway functionality. Key changes include updates to the Changes
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: 2
🧹 Outside diff range and nitpick comments (3)
pkg/contracts/solana/gateway.go (2)
Line range hint
1-1: Fix typo in package documentation.The package documentation contains a typo: "privides" should be "provides".
-// Package solana privides structures and constants that are used when interacting with the gateway program on Solana chain. +// Package solana provides structures and constants that are used when interacting with the gateway program on Solana chain.
Line range hint
17-23: Fix duplicate constant documentation.The comment "AccountsNumberOfDeposit" is duplicated. The second instance should be "AccountsNumberOfDepositSPL" to match its constant.
- // AccountsNumberOfDeposit is the number of accounts required for Solana gateway deposit spl instruction + // AccountsNumberOfDepositSPL is the number of accounts required for Solana gateway deposit spl instructionpkg/contracts/solana/gateway.json (1)
426-468: Review the new set_deposit_fee instruction.The new instruction allows setting a deposit fee with appropriate account permissions:
pdais correctly marked as writable since it stores the feesigneris correctly marked as writable and requires signature- The
feeparameter is appropriately typed asu64However, there's a potential security consideration:
Consider adding validation to prevent setting unreasonably high fees that could effectively block deposits. This could be done by:
- Adding a maximum fee constant
- Adding fee validation logic in the instruction processor
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
contrib/localnet/solana/gateway.sois excluded by!**/*.so
📒 Files selected for processing (3)
e2e/runner/solana.go(1 hunks)pkg/contracts/solana/gateway.go(1 hunks)pkg/contracts/solana/gateway.json(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
pkg/contracts/solana/gateway.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/solana.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (5)
e2e/runner/solana.go (2)
119-119: Verify System Program account ordering and requirements
The addition of the System Program ID to the account metadata suggests system-level operations. Let's verify:
- The account ordering matches the Solana program expectations
- The System Program is required for the intended operations
✅ Verification successful
System Program account ordering is correctly implemented
The verification confirms that the System Program account is appropriately placed and required in the instruction for system-level operations. The analysis shows:
- The System Program is consistently used in deposit and whitelist operations
- Account ordering follows Solana's conventions where:
- Writable signer accounts are placed first
- Other writable accounts follow
- System Program is correctly placed last as a read-only account
The implementation in e2e/runner/solana.go aligns with Solana's best practices for account ordering and system program integration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify System Program usage and account ordering
# Search for System Program usage patterns
ast-grep --pattern 'system_program::ID'
# Look for account validation in the program
rg -A 10 'accounts\s*:.*SystemProgram'
Length of output: 80
Script:
#!/bin/bash
# Let's try different approaches to verify System Program usage
# Look for SystemProgram references in the codebase
rg -i "systemprogram"
# Search for solana.Meta patterns to understand account ordering
rg -B 5 -A 5 "solana\.Meta"
# Look for account validation or instruction context
rg -i "system.*program.*id"
# Check the specific file content for context
cat "e2e/runner/solana.go"
Length of output: 22137
113-113: Verify PDA write permissions against program requirements
The addition of write permissions to the PDA account is a critical change. Let's verify this against the Solana program requirements.
✅ Verification successful
PDA write permission is correctly implemented
The verification confirms that setting the PDA account as writable in the deposit instruction is consistent with the codebase's patterns and requirements:
- The same PDA write permission pattern is used across multiple instruction types (deposit, deposit_spl, withdraw)
- The pattern is consistently implemented in both production code (zetaclient/chains/solana/) and test code (e2e/runner/)
- The account structure follows the Solana program requirements where:
accountsNumDeposit = 3(signer, pda, system_program)- PDA is the second account and requires write permission
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify PDA write permissions in the Solana program
# Look for account permission definitions in the Solana program
# Search for PDA account definitions and their permissions
ast-grep --pattern 'AccountInfo<$_> { is_writable: $_, .. }'
# Search for explicit PDA write permission requirements
rg -A 5 'require.*is_writable'
Length of output: 95
Script:
#!/bin/bash
# Let's try a different approach to verify PDA write permissions
# Search for PDA-related code and write permissions in Solana files
rg -g "*.{rs,go}" -i "pda.*write|write.*pda" -A 3 -B 3
# Look for account metadata configurations in the test runner
rg -g "*.go" "solana\.Meta.*WRITE" -A 2 -B 2
# Search for deposit-related instruction definitions
rg -g "*.{rs,go}" "deposit.*instruction|instruction.*deposit" -A 5
Length of output: 22036
pkg/contracts/solana/gateway.json (3)
1089-1093: Approve new error code for empty receiver validation.
The new EmptyReceiver error with code 6010 is well-defined and follows the existing error code pattern.
Line range hint 146-203: Verify account permissions in deposit instructions.
The changes to deposit instructions include making the pda and to accounts writable and adding the system_program. While these changes are necessary for deposit fee functionality:
- The
system_programaddition suggests native SOL transfers for fees - Making
towritable indicates it will be modified during the deposit
Let's verify the account permissions are consistent with other similar instructions:
Also applies to: 242-299
✅ Verification successful
Account permissions are correctly configured for deposit fee functionality
The verification shows consistent patterns across the contract interface:
- The
toaccount is marked as writable in all deposit-related instructions, which is necessary since it will receive tokens - The
pdaaccount is consistently marked as writable across all instructions where it's used - The
system_programis properly referenced with the correct address (11111111111111111111111111111111) in all instructions that require native SOL transfers
The changes align with Solana's account model where accounts that will be modified during instruction execution must be marked as writable, and the system program is required for handling native SOL transfers for fees.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in account permissions across similar instructions
# Look for other instructions that interact with these accounts
# Search for account definitions with similar patterns
rg -A 5 '"name": "to",'
rg -A 5 '"name": "pda",'
# Search for system_program usage patterns
rg -A 5 '"name": "system_program",'
Length of output: 12744
1126-1133: Review account structure updates.
The changes to account structures are consistent:
- Added
deposit_fee(u64) toPdafor fee storage - Added
bump(u8) to all account structures for PDA derivation
Let's verify these changes are used consistently:
Also applies to: 1142-1147, 1154-1159
✅ Verification successful
The previous attempt with ast-grep didn't yield results, likely because the pattern syntax wasn't optimal for Rust code or the files weren't recognized as the correct language. Let's try using ripgrep to search for these field names in the codebase, with some context lines to understand their usage patterns.
If this doesn't provide enough context, we can follow up with more specific searches or examine the instruction handlers directly.
The previous attempts failed because ripgrep couldn't find Rust files. Since we're dealing with a Solana contract that might use different file extensions, let's first list all files and then search without type filtering. We're also adjusting the bump search pattern to look for the field name in quotes as it appears in the JSON structure.
Account structure changes verified in gateway.json
The search results confirm consistent implementation of the account structure changes:
deposit_fee(u64) is present in the PDA structurebump(u8) field is consistently added to all relevant account structures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of new fields
# Look for references to these fields in instruction handlers
# Search for deposit_fee field usage
ast-grep --pattern 'pda.deposit_fee'
# Search for bump field usage
ast-grep --pattern '$_.bump'
Length of output: 67
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to search for these fields
# Search for deposit_fee references
rg -U "deposit_fee" --type rust -A 3 -B 3
# Search for bump references in account structures and their usage
rg -U "bump.*[ui][0-9]+" --type rust -A 3 -B 3
Length of output: 388
Script:
#!/bin/bash
# Let's search without type filtering since the file extension might not be .rs
# First, find all relevant files
fd -t f
# Then search for the fields in all files
rg -U "deposit_fee" -A 3 -B 3
# Search for bump field references
rg -U "\"bump\"" -A 3 -B 3
Length of output: 87538
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3232 +/- ##
===========================================
+ Coverage 61.80% 61.81% +0.01%
===========================================
Files 431 431
Lines 30748 30759 +11
===========================================
+ Hits 19004 19015 +11
Misses 10886 10886
Partials 858 858
|
* integrate deposit fee and audit fixes * unit test fix * latest gateway changes * latest gateway * latest gateway
Description
Integrate changes from this PR: zeta-chain/protocol-contracts-solana#66
How Has This Been Tested?
Summary by CodeRabbit
New Features
set_deposit_feeto enhance fee management.system_programaccount and a writabletoaccount.Improvements
system_program.Bug Fixes
6010for handling empty receiver scenarios.