Adding execute rust test and fixing freeze_execute test #238
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Summary by CodeRabbit
WalkthroughA new integration test module was added to verify the full execution flow of the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant Solana as Solana Test Env
participant Payer as Payer Account
participant Asset as Asset Account (PDA)
participant System as System Program
Test->>Solana: Initialize test environment
Test->>Payer: Fund payer account
Test->>Solana: Mint new asset
Payer->>Asset: Transfer 0.5 SOL to asset signer PDA
Test->>System: Construct ExecuteV1 instruction (transfer lamports from PDA to payer)
System->>Asset: Transfer all lamports from asset signer PDA to payer
Test->>Asset: Verify asset signer PDA balance is zero
Test->>Payer: Verify payer received lamports
sequenceDiagram
participant Test as Integration Test
participant Payer as Payer Account
participant Asset as Asset Signer PDA
participant System as System Program
Test->>Asset: Compute PDA for asset signer
Payer->>Asset: Transfer 0.5 SOL to PDA
Test->>Asset: Query PDA balance (should be 0.5 SOL)
Test->>System: Attempt ExecuteV1 (transfer lamports from PDA to payer)
System-->>Test: Fail with InvalidAuthority (plugin frozen)
Test->>Asset: Query PDA balance (should remain unchanged)
Test->>Payer: Burn asset, verify payer's balance increased
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
clients/rust/tests/execute.rs(1 hunks)clients/rust/tests/freeze_execute.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
clients/rust/tests/freeze_execute.rs (4)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:81-83
Timestamp: 2025-06-24T20:46:47.342Z
Learning: In the add_assets_to_group_v1 processor (programs/mpl-core/src/processor/add_assets_to_group.rs), the payer must always be a signer even when an explicit authority is provided, because the payer is responsible for paying rent for new data space when assets are added to groups.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:99-103
Timestamp: 2025-06-24T20:50:30.534Z
Learning: In the mpl-core codebase, writable checks are generally ignored because the Solana runtime will automatically throw an error if a readonly account is written to, making explicit writable checks redundant.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/instruction.rs:48-68
Timestamp: 2025-06-05T12:01:30.794Z
Learning: In the mpl-core program, new variants must always be appended to the end of the MplAssetInstruction enum to maintain on-chain ABI compatibility. Inserting variants in the middle breaks Borsh serialization ordinals and would render existing clients unable to decode instructions correctly.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:96-105
Timestamp: 2025-06-05T12:01:33.707Z
Learning: Solana accounts are currently limited to 10MiB (10,485,760 bytes), and u32::MAX-sized accounts (~4.3GB) would cost approximately 30K SOL in rent, making them financially infeasible. Therefore, integer overflow protection in length calculations is generally unnecessary for Solana program account size computations due to these practical runtime constraints.
🧬 Code Graph Analysis (2)
clients/rust/tests/execute.rs (1)
clients/rust/tests/setup/mod.rs (4)
program_test(15-17)airdrop(363-381)create_asset(40-73)assert_asset(86-191)
clients/rust/tests/freeze_execute.rs (1)
programs/mpl-core/src/processor/transfer.rs (1)
transfer(22-144)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Programs / Build
🔇 Additional comments (5)
clients/rust/tests/execute.rs (1)
1-142: Well-structured integration test for ExecuteV1 instruction.The test implementation follows good practices with clear sections, proper error handling, and comprehensive assertions. The flow correctly demonstrates the full lifecycle of asset execution with SOL backing.
clients/rust/tests/freeze_execute.rs (4)
80-83: PDA derivation correctly updated.The switch to using a program-derived address for the asset signer is implemented correctly and aligns with the approach used in execute.rs.
86-86: SOL deposit and balance verification properly updated.The transfer instruction and balance checks have been correctly updated to use the asset_signer PDA instead of the asset pubkey directly. The assertions properly verify the deposit was successful.
Also applies to: 96-103
121-133: ExecuteV1Builder properly configured with instruction data and accounts.The addition of
instruction_dataandadd_remaining_accountscorrectly provides the necessary information for the system transfer instruction that the ExecuteV1 instruction will attempt to execute. This matches the pattern used in execute.rs.
172-180: Post-execution balance assertions correctly updated.The balance verification properly checks the asset_signer PDA and confirms that the SOL remains in the PDA after the failed execution (due to the frozen plugin), which is the expected behavior.
| use solana_program_test::tokio; | ||
| use solana_sdk::{signature::Keypair, signer::Signer, transaction::Transaction}; | ||
|
|
||
| const EXECUTE_PREFIX: &str = "mpl-core-execute"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using a shared constant for PDA prefix.
The constant EXECUTE_PREFIX uses the same value as FREEZE_EXECUTE_PREFIX in the freeze_execute.rs file. Consider extracting this to a shared module to maintain consistency and avoid duplication.
You could create a shared constants module:
// In a shared module (e.g., setup/constants.rs)
+pub const EXECUTE_PREFIX: &str = "mpl-core-execute";
// Then import it in both files
+use setup::constants::EXECUTE_PREFIX;
-const EXECUTE_PREFIX: &str = "mpl-core-execute";Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In clients/rust/tests/execute.rs at line 14, the constant EXECUTE_PREFIX
duplicates the value of FREEZE_EXECUTE_PREFIX from freeze_execute.rs. To fix
this, create a shared constants module accessible by both files, move the common
prefix constant there, and update both files to import and use the shared
constant instead of defining their own.
| let execute_transfer_ix = | ||
| system_instruction::transfer(&asset_signer, &payer_key, lamports_in_asset); | ||
|
|
||
| println!("execute_transfer_ix: {:#?}", execute_transfer_ix); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove debug print statement.
The debug print appears to be left over from development/debugging and should be removed before merging.
- println!("execute_transfer_ix: {:#?}", execute_transfer_ix);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| println!("execute_transfer_ix: {:#?}", execute_transfer_ix); |
🤖 Prompt for AI Agents
In clients/rust/tests/freeze_execute.rs at line 111, remove the debug print
statement that outputs the execute_transfer_ix variable. This println! line was
used for debugging and should be deleted to clean up the code before merging.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Benchmark
| Benchmark suite | Current: 30b2980 | Previous: 7e40184 | Ratio |
|---|---|---|---|
CU: create a new, empty asset |
9812 Compute Units |
9812 Compute Units |
1 |
Space: create a new, empty asset |
91 Bytes |
91 Bytes |
1 |
CU: create a new, empty asset with empty collection |
21290 Compute Units |
21290 Compute Units |
1 |
Space: create a new, empty asset with empty collection |
91 Bytes |
91 Bytes |
1 |
CU: create a new asset with plugins |
31004 Compute Units |
31004 Compute Units |
1 |
Space: create a new asset with plugins |
194 Bytes |
194 Bytes |
1 |
CU: create a new asset with plugins and empty collection |
36655 Compute Units |
36655 Compute Units |
1 |
Space: create a new asset with plugins and empty collection |
194 Bytes |
194 Bytes |
1 |
CU: list an asset |
27406 Compute Units |
27406 Compute Units |
1 |
CU: sell an asset |
41771 Compute Units |
41771 Compute Units |
1 |
CU: list an asset with empty collection |
35561 Compute Units |
35561 Compute Units |
1 |
CU: sell an asset with empty collection |
55047 Compute Units |
55047 Compute Units |
1 |
CU: list an asset with collection royalties |
36875 Compute Units |
36875 Compute Units |
1 |
CU: sell an asset with collection royalties |
60830 Compute Units |
60830 Compute Units |
1 |
CU: transfer an empty asset |
5262 Compute Units |
5262 Compute Units |
1 |
CU: transfer an empty asset with empty collection |
8036 Compute Units |
8036 Compute Units |
1 |
CU: transfer an asset with plugins |
11483 Compute Units |
11483 Compute Units |
1 |
CU: transfer an asset with plugins and empty collection |
14257 Compute Units |
14257 Compute Units |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
No description provided.