Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

rpc: add tests for simulate transaction inner instructions #34495

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

buffalojoec
Copy link
Contributor

Problem

As of #34313, inner instructions can now optionally be included in the RPC response for simulateTransaction. However, the test coverage on this new feature is lacking.

Summary of Changes

Added some tests specifically for the innerInstructions on the RPC's simulateTransaction method.

Copy link

codecov bot commented Dec 16, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (a5c470d) 81.7% compared to head (8ec37ad) 81.7%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #34495    +/-   ##
========================================
  Coverage    81.7%    81.7%            
========================================
  Files         826      826            
  Lines      223413   223458    +45     
========================================
+ Hits       182614   182721   +107     
+ Misses      40799    40737    -62     

@github-actions github-actions bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Jan 1, 2024
@buffalojoec buffalojoec added the do-not-close Add this tag to exempt an issue/PR from being closed by the stalebot label Jan 3, 2024
@buffalojoec buffalojoec force-pushed the rpc-simulate-inner-tests branch from 2552c36 to 9c1e45f Compare January 3, 2024 16:07
@buffalojoec
Copy link
Contributor Author

Ah nuts, I think some recent Bank changes nuked this test setup.

@buffalojoec buffalojoec force-pushed the rpc-simulate-inner-tests branch from 9c1e45f to d33b2b7 Compare January 5, 2024 17:23
@buffalojoec
Copy link
Contributor Author

This should be ready for another look @CriesofCarrots.

Is there a way to trigger CI manually for PRs? I think the "clippy on MacOS" job hiccuped, but I seem to be compliant here.

I can push a rebase sometime in the future as well to trigger if need be.

@steveluscher
Copy link
Contributor

steveluscher commented Jan 20, 2024

So excited for this! If you get a chance to add this to the docs too… 🤌🏻

solana-foundation/developer-content#46

@buffalojoec buffalojoec force-pushed the rpc-simulate-inner-tests branch from d33b2b7 to 0b65a58 Compare January 21, 2024 04:33
@buffalojoec
Copy link
Contributor Author

I rewrote this to use built-in program instructions to avoid the added dependencies and setup stuff. Much cleaner now.

If we want coverage on the token program instructions, etc. We'll have to consider adding program-test or at least the .so files of the SPL programs we want to test.

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.

Nice, much lighter, thanks!
Just a couple suggestions for simplification, and one question

rpc/src/rpc.rs Outdated Show resolved Hide resolved
rpc/src/rpc.rs Outdated Show resolved Hide resolved
rpc/src/rpc.rs Outdated Show resolved Hide resolved
rpc/src/rpc.rs Outdated Show resolved Hide resolved
rpc/src/rpc.rs Outdated Show resolved Hide resolved
rpc/src/rpc.rs Outdated Show resolved Hide resolved
rpc/src/rpc.rs Outdated Show resolved Hide resolved
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+ once CI is happy; you'll need to rebase to pick up the audit fix

rpc/src/rpc.rs Outdated Show resolved Hide resolved
@buffalojoec buffalojoec force-pushed the rpc-simulate-inner-tests branch from 7fafd1f to 8ec37ad Compare January 22, 2024 21:55
@CriesofCarrots
Copy link
Contributor

And yes, please update the JSON-RPC docs! 🙏 (I know I'm going to have a hard time remembering to update them now that they're out of this repo. Hopefully we can help each other :))

@buffalojoec
Copy link
Contributor Author

And yes, please update the JSON-RPC docs! 🙏 (I know I'm going to have a hard time remembering to update them now that they're out of this repo. Hopefully we can help each other :))

I put it on my list!

@buffalojoec buffalojoec merged commit 9caf9e8 into solana-labs:master Jan 23, 2024
35 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

3 participants