Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RPC: Add inner instructions to simulate transaction response #34313

Merged
merged 10 commits into from
Dec 16, 2023

Conversation

buffalojoec
Copy link
Contributor

@buffalojoec buffalojoec commented Dec 5, 2023

Problem

Wallets use transaction simulation to notify users about the contents of a transaction before they apply their signature. This is the primary method by which wallets, and potentially other dApps and infrastructure, detect malicious instructions.

Currently, transaction simulation offers the ability to observe the resulting account state for various account keys provided and transaction logs. However, it would provide even more enhanced security for Solana users if transaction simulation also returned the parsed inner instructions of a transaction.

Summary of Changes

The RPC change includes adding an optional parameter - innerInstructions: bool - to simulateTransaction, and adding jsonParsed inner instructions to the response type if innerInstructions is provided as true. Similar to getTransaction, if the instruction cannot be parsed with jsonParsed, it defaults to json, where the data field is base58 encoded.

This change also required the bank to return inner_instructions from its simulate_transaction method.

@buffalojoec buffalojoec force-pushed the solflare-rpc-simulate branch 3 times, most recently from 8b59c17 to 573b827 Compare December 5, 2023 16:04
@buffalojoec buffalojoec marked this pull request as ready for review December 5, 2023 16:41
@buffalojoec buffalojoec force-pushed the solflare-rpc-simulate branch 2 times, most recently from b726d73 to a888af0 Compare December 5, 2023 16:42
@buffalojoec buffalojoec force-pushed the solflare-rpc-simulate branch 3 times, most recently from 3c1e30f to c24d74f Compare December 6, 2023 01:05
@buffalojoec buffalojoec added the do-not-close Add this tag to exempt an issue/PR from being closed by the stalebot label Dec 13, 2023
@buffalojoec buffalojoec force-pushed the solflare-rpc-simulate branch 3 times, most recently from e974a2a to ad2e601 Compare December 15, 2023 16:32
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! One attribute request, plus adding the conversion helper fn
And CI being happy, ofc

accounts-db/src/transaction_results.rs Show resolved Hide resolved
rpc/src/rpc.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 15, 2023

Codecov Report

Merging #34313 (2d5f197) into master (310c7a4) will decrease coverage by 0.1%.
Report is 1 commits behind head on master.
The diff coverage is 79.1%.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34313     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         819      820      +1     
  Lines      220889   220905     +16     
=========================================
- Hits       180845   180782     -63     
- Misses      40044    40123     +79     

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+ one last nit!

accounts-db/src/transaction_results.rs Outdated Show resolved Hide resolved
@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Dec 16, 2023
@solana-grimes solana-grimes removed the automerge Merge this Pull Request automatically once CI passes label Dec 16, 2023
@solana-grimes
Copy link
Collaborator

😱 New commits were pushed while the automerge label was present.

@buffalojoec buffalojoec added the automerge Merge this Pull Request automatically once CI passes label Dec 16, 2023
@mergify mergify bot merged commit 171c58c into solana-labs:master Dec 16, 2023
46 checks passed
@buffalojoec buffalojoec removed the automerge Merge this Pull Request automatically once CI passes label Dec 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-close Add this tag to exempt an issue/PR from being closed by the stalebot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants