Conversation
📝 WalkthroughWalkthroughThe PR adds returning post-simulation account state to transaction simulations. It extends TransactionSimulationResult with a post_simulation_accounts field, populates that field in the processor's simulate path (including executed transactions), and updates the HTTP simulate handler to resolve, validate, encode, and return requested accounts (merging post-simulation over current accounts). A new test asserts that requested accounts are returned with expected balances and entries. Assessment against linked issues
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4128974682
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d18a9da12a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-aperture/src/requests/http/simulate_transaction.rs`:
- Around line 77-82: The error message returned when rejecting unsupported
encodings is inaccurate: the conditional checks for UiAccountEncoding::Binary or
UiAccountEncoding::Base58 but the RpcError::invalid_params message only mentions
"base58 encoding not supported"; update the error text in the
RpcError::invalid_params call (in the conditional that inspects
accounts_encoding) to reflect both unsupported encodings (e.g., "binary and
base58 encodings not supported" or similar) so the message matches the checked
UiAccountEncoding variants.
In `@magicblock-processor/src/executor/processing.rs`:
- Around line 120-139: The current branch handling
ProcessedTransaction::Executed only collects
executed.loaded_transaction.accounts up to number_of_accounts, which omits any
additionally requested pubkeys and forces callers to patch accounts later;
update the code that builds the simulation request to include the requested
pubkeys (e.g., a field like simulation_request.requested_pubkeys) and here in
the match arm for ProcessedTransaction::Executed materialize those requested
accounts from the same simulated snapshot instead of only using
executed.loaded_transaction.accounts — replace the post_simulation_accounts =
executed.loaded_transaction.accounts.into_iter().take(number_of_accounts).collect()
logic with code that merges/constructs accounts by pulling each requested pubkey
from the snapshot (using the same snapshot source used by the simulator) so the
returned accounts array contains the post-simulation state for all requested
addresses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 20290f3d-56f4-49bb-9203-9682e60705fd
📒 Files selected for processing (4)
magicblock-aperture/src/requests/http/simulate_transaction.rsmagicblock-aperture/tests/transactions.rsmagicblock-core/src/link/transactions.rsmagicblock-processor/src/executor/processing.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 638b33ed6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
bmuddha
left a comment
bmuddha
left a comment
There was a problem hiding this comment.
LGTM, with couple minor nits
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7112f30cc8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| post_simulation_accounts | ||
| .get(&pubkey) | ||
| .cloned() | ||
| .or(account) |
There was a problem hiding this comment.
When a requested address is not present in post_simulation_accounts, the code falls back to read_accounts_with_ensure via .or(account), so simulateTransaction can return a regular current account snapshot for an address that was never part of the simulated message. This makes the accounts field report non-simulated state as if it were post-simulation output (and can also trigger unnecessary account fetches) instead of failing fast for out-of-scope addresses. Validate each requested address against the transaction account keys before building the response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-aperture/src/requests/http/simulate_transaction.rs`:
- Around line 55-125: Validate config.accounts (encoding, address count, and
parse each address into Pubkey) before calling
self.transactions_scheduler.simulate(...) so malformed params or unsupported
encodings are rejected prior to consuming scheduler work; then after simulation
use the returned TransactionSimulationResult.post_simulation_accounts map to
assemble the response for each requested pubkey (mapping missing entries to
None) instead of calling read_accounts_with_ensure (remove the fallback to
current AccountsDb), and preserve the existing behavior of returning
Some(vec![None; ...]) only when result_err.is_some() after validation and
parsing have already occurred.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a6eb3e83-fbc3-4028-ad51-481250e169e4
📒 Files selected for processing (2)
magicblock-aperture/src/requests/http/simulate_transaction.rsmagicblock-core/src/link/transactions.rs
| let inner_instructions_enabled = config.inner_instructions; | ||
| let accounts_config = config.accounts; | ||
|
|
||
| // Submit the transaction to the scheduler for simulation. | ||
| let result = self | ||
| .transactions_scheduler | ||
| .simulate(transaction.txn) | ||
| .await | ||
| .map_err(RpcError::transaction_simulation)?; | ||
| let TransactionSimulationResult { | ||
| result, | ||
| logs, | ||
| post_simulation_accounts, | ||
| units_consumed, | ||
| return_data, | ||
| inner_instructions: recorded_inner_instructions, | ||
| } = result; | ||
| let result_err = result.as_ref().err().cloned(); | ||
| let accounts = if let Some(config_accounts) = accounts_config { | ||
| let accounts_encoding = config_accounts | ||
| .encoding | ||
| .unwrap_or(UiAccountEncoding::Base64); | ||
|
|
||
| if accounts_encoding == UiAccountEncoding::Binary | ||
| || accounts_encoding == UiAccountEncoding::Base58 | ||
| { | ||
| return Err(RpcError::invalid_params( | ||
| "base58 encoding not supported", | ||
| )); | ||
| } | ||
|
|
||
| if config_accounts.addresses.len() > number_of_accounts { | ||
| return Err(RpcError::invalid_params(format!( | ||
| "Too many accounts provided; max {number_of_accounts}" | ||
| ))); | ||
| } | ||
|
|
||
| if result_err.is_some() { | ||
| Some(vec![None; config_accounts.addresses.len()]) | ||
| } else { | ||
| let pubkeys = config_accounts | ||
| .addresses | ||
| .into_iter() | ||
| .map(|address| { | ||
| address | ||
| .parse::<Pubkey>() | ||
| .map_err(RpcError::invalid_params) | ||
| }) | ||
| .collect::<Result<Vec<_>, _>>()?; | ||
| let current_accounts = | ||
| self.read_accounts_with_ensure(&pubkeys).await; | ||
| let post_simulation_accounts = post_simulation_accounts | ||
| .into_iter() | ||
| .collect::<HashMap<_, _>>(); | ||
|
|
||
| Some( | ||
| pubkeys | ||
| .into_iter() | ||
| .zip(current_accounts) | ||
| .map(|(pubkey, account)| { | ||
| post_simulation_accounts | ||
| .get(&pubkey) | ||
| .cloned() | ||
| .or(account) | ||
| .map(|account| { | ||
| LockedAccount::new(pubkey, account) | ||
| .ui_encode(accounts_encoding, None) | ||
| }) | ||
| }) | ||
| .collect(), | ||
| ) |
There was a problem hiding this comment.
Move config.accounts validation ahead of the simulation call.
Lines 59-63 run the simulation before any config.accounts checks. That lets unsupported encodings, oversized address lists, and malformed pubkeys consume scheduler work, and it is why Line 92 can return Some(vec![None; ...]) without ever parsing the supplied addresses. Also, magicblock-processor/src/executor/processing.rs:128-133 already records all simulated transaction accounts, so a miss in Lines 104-123 means the caller asked for a non-simulated pubkey; falling back to read_accounts_with_ensure then returns current AccountsDb state instead of simulation state.
Suggested restructuring
let replacement_blockhash = config
.replace_recent_blockhash
.then(|| RpcBlockhash::from(self.blocks.get_latest()));
let inner_instructions_enabled = config.inner_instructions;
- let accounts_config = config.accounts;
+ let requested_accounts = if let Some(config_accounts) = config.accounts {
+ let accounts_encoding = config_accounts
+ .encoding
+ .unwrap_or(UiAccountEncoding::Base64);
+
+ if matches!(
+ accounts_encoding,
+ UiAccountEncoding::Binary | UiAccountEncoding::Base58
+ ) {
+ return Err(RpcError::invalid_params(
+ "base58 encoding not supported",
+ ));
+ }
+
+ let pubkeys = config_accounts
+ .addresses
+ .into_iter()
+ .map(|address| {
+ address
+ .parse::<Pubkey>()
+ .map_err(RpcError::invalid_params)
+ })
+ .collect::<Result<Vec<_>, _>>()?;
+
+ if pubkeys.len() > number_of_accounts {
+ return Err(RpcError::invalid_params(format!(
+ "Too many accounts provided; max {number_of_accounts}"
+ )));
+ }
+
+ let tx_account_keys = transaction.txn.message().account_keys();
+ if pubkeys.iter().any(|pubkey| {
+ !tx_account_keys.iter().any(|account_key| account_key == pubkey)
+ }) {
+ return Err(RpcError::invalid_params(
+ "requested accounts must be present in the transaction",
+ ));
+ }
+
+ Some((accounts_encoding, pubkeys))
+ } else {
+ None
+ };
let result = self
.transactions_scheduler
.simulate(transaction.txn)
.await
.map_err(RpcError::transaction_simulation)?;
@@
- let accounts = if let Some(config_accounts) = accounts_config {
- let accounts_encoding = config_accounts
- .encoding
- .unwrap_or(UiAccountEncoding::Base64);
-
- if accounts_encoding == UiAccountEncoding::Binary
- || accounts_encoding == UiAccountEncoding::Base58
- {
- return Err(RpcError::invalid_params(
- "base58 encoding not supported",
- ));
- }
-
- if config_accounts.addresses.len() > number_of_accounts {
- return Err(RpcError::invalid_params(format!(
- "Too many accounts provided; max {number_of_accounts}"
- )));
- }
-
+ let accounts = if let Some((accounts_encoding, pubkeys)) = requested_accounts {
if result_err.is_some() {
- Some(vec![None; config_accounts.addresses.len()])
+ Some(vec![None; pubkeys.len()])
} else {
- let pubkeys = config_accounts
- .addresses
- .into_iter()
- .map(|address| {
- address
- .parse::<Pubkey>()
- .map_err(RpcError::invalid_params)
- })
- .collect::<Result<Vec<_>, _>>()?;
- let current_accounts =
- self.read_accounts_with_ensure(&pubkeys).await;
let post_simulation_accounts = post_simulation_accounts
.into_iter()
.collect::<HashMap<_, _>>();
Some(
pubkeys
.into_iter()
- .zip(current_accounts)
- .map(|(pubkey, account)| {
+ .map(|pubkey| {
post_simulation_accounts
.get(&pubkey)
.cloned()
- .or(account)
.map(|account| {
LockedAccount::new(pubkey, account)
.ui_encode(accounts_encoding, None)
})
})
.collect(),
)
}
} else {
None
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-aperture/src/requests/http/simulate_transaction.rs` around lines
55 - 125, Validate config.accounts (encoding, address count, and parse each
address into Pubkey) before calling self.transactions_scheduler.simulate(...) so
malformed params or unsupported encodings are rejected prior to consuming
scheduler work; then after simulation use the returned
TransactionSimulationResult.post_simulation_accounts map to assemble the
response for each requested pubkey (mapping missing entries to None) instead of
calling read_accounts_with_ensure (remove the fallback to current AccountsDb),
and preserve the existing behavior of returning Some(vec![None; ...]) only when
result_err.is_some() after validation and parsing have already occurred.
2 participants
Summary
Compatibility
Testing
Checklist
Summary by CodeRabbit