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: parse token accounts in simulate_transaction #34619

Merged
merged 8 commits into from
Jan 19, 2024

Conversation

yihau
Copy link
Member

@yihau yihau commented Jan 2, 2024

Problem

jsonParsed will never work for token accounts because we missed decimals.

Summary of Changes

add new parsing code for JsonParsed. it will

- check if accounts's owner is token program
- get mint account data from either post_simulation_accounts or bank and decimals from it.

updated:

add a new func arg, overwrite_accounts, to fn get_encoded_account and fn get_parsed_token_account.

@yihau yihau marked this pull request as ready for review January 2, 2024 12:46
Copy link

codecov bot commented Jan 2, 2024

Codecov Report

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

Comparison is base (e2c2029) 81.7% compared to head (6e60b1a) 81.8%.
Report is 1 commits behind head on master.

❗ Current head 6e60b1a differs from pull request most recent head 6407b27. Consider uploading reports for the commit 6407b27 to get more accurate results

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #34619    +/-   ##
========================================
  Coverage    81.7%    81.8%            
========================================
  Files         825      825            
  Lines      223261   222316   -945     
========================================
- Hits       182605   181955   -650     
+ Misses      40656    40361   -295     

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.

I'm on board with this change conceptually (in fact, I'm a little surprised no one's complained about the issue before now), but I don't love how much it duplicates code that already exists in some fashion.

Let's articulate the reasons solana_rpc::get_encoded_account doesn't work for this use, and maybe we can rework it into a submethod or two that reuses the existing encoding logic?

@yihau
Copy link
Member Author

yihau commented Jan 4, 2024

just updated with a new attempt. I passed a new variable overwrite_accounts into those parsing functions.

@mcintyre94
Copy link
Contributor

I opened the issue for this yesterday, forgot to check for an open PR first sorry! One reason it hasn't been complained about before is that the legacy web3js doesn't expose the accounts encoding option, so everyone there has been stuck with base64 on all accounts. I came across this issue building with the new web3js which exposes the accounts encoding.

I've also mentioned the issue to @joncinque and he suggested that we might be able to backport this to 1.17, since it only affects the JSON RPC and is a bug. If that is possible then it'd be really helpful!

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.

This looks really close now! Just some thoughts about naming and organizing the new get_account method.

rpc/src/common.rs Outdated Show resolved Hide resolved
rpc/src/rpc.rs Show resolved Hide resolved
rpc/src/lib.rs Outdated Show resolved Hide resolved
rpc/src/rpc.rs Show resolved Hide resolved
@yihau yihau force-pushed the simulate-tx-account-decoding branch from 54a7d9c to 50cba61 Compare January 17, 2024 13:33
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.

One last comment nit; I made it as a suggestion so it is easy to commit. Then lgtm!

rpc/src/rpc.rs Show resolved Hide resolved
rpc/src/rpc.rs Show resolved Hide resolved
@CriesofCarrots CriesofCarrots added the v1.17 PRs that should be backported to v1.17 label Jan 18, 2024
Copy link
Contributor

mergify bot commented Jan 18, 2024

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

@yihau yihau force-pushed the simulate-tx-account-decoding branch from 7ec7687 to 6407b27 Compare January 18, 2024 16:24
@yihau yihau merged commit 7470c3d into solana-labs:master Jan 19, 2024
33 of 35 checks passed
@yihau yihau deleted the simulate-tx-account-decoding branch January 19, 2024 03:53
mergify bot pushed a commit that referenced this pull request Jan 19, 2024
* rpc: parse token accounts in simulate_transaction

* add overwrite_accounts into get_encoded_account and get_parsed_token_account

* revert get_mint_decimals scope changes

* move common.rs to rpc/account_resolver.rs

* rename get_account to get_account_from_overwrites_or_bank

* add a comment

* clippy

* add comment

Co-authored-by: Tyera <teulberg@gmail.com>

---------

Co-authored-by: Tyera <teulberg@gmail.com>
(cherry picked from commit 7470c3d)
CriesofCarrots pushed a commit that referenced this pull request Jan 20, 2024
…34619) (#34852)

* rpc: parse token accounts in simulate_transaction (#34619)

* rpc: parse token accounts in simulate_transaction

* add overwrite_accounts into get_encoded_account and get_parsed_token_account

* revert get_mint_decimals scope changes

* move common.rs to rpc/account_resolver.rs

* rename get_account to get_account_from_overwrites_or_bank

* add a comment

* clippy

* add comment

Co-authored-by: Tyera <teulberg@gmail.com>

---------

Co-authored-by: Tyera <teulberg@gmail.com>
(cherry picked from commit 7470c3d)

* remove innerInstructions

---------

Co-authored-by: Yihau Chen <a122092487@gmail.com>
Co-authored-by: yihau <yihau.chen@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.17 PRs that should be backported to v1.17
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants