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

Show token balance changes on a simulated transaction #286

Merged
merged 6 commits into from
Sep 14, 2023

Conversation

brittcyr
Copy link
Contributor

@brittcyr brittcyr commented Aug 21, 2023

Currently, the simulation only shows whether or not the transaction succeeds and the logs.

It would be very useful for this to include token balance changes since the token program doesnt log the accounts or amounts. This should bring the displayed information of a simulation up to parity with what the explorer does for completed transactions.

The difficult part of this is that the simulateTransaction RPC spec does not include the pre/post token balances similar to asking for a completed transaction. To address this, we request the accountData for all accounts immediately before simulating and record the values of all token accounts. Then the simulateTransaction allows optional arguments for accounts to get their post simulation data. Parse that to get the post transaction token balance. Then convert it to the same format that the existing token balances component from the transaction page.

Example with token balances (before, the simulate was the same thing without the token changes component):
image

Copy link

@asktree asktree left a comment

Choose a reason for hiding this comment

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

commented on telegram. this approach doesn't smell right to me, getting the explorer to show the right info should not be very complicated.

EDIT:
OK, the problem is that I thought this was a governance-ui PR. now I understand my confusion. please ignore.

@DonDuala
Copy link

@ngundotra can you review for us pls

Copy link
Collaborator

@mcintyre94 mcintyre94 left a comment

Choose a reason for hiding this comment

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

Hey @brittcyr, thanks for contributing this, this is a great idea to add!

I do have one general concern/question about this approach, but I'm not sure there's a better alternative. In general for past transactions, the current balance isn't going to be an accurate pre-transaction balance. Do you have any thoughts about the impact of that?

I think this is going to be much more useful for a proposed transaction than for inspecting a past transaction. Do you think it'd make sense to disable this for the /tx/<id>/inspect page, and only use it when inspecting a transaction from base64? I'm curious what you're finding this most useful for though! And obviously nothing stops someone dumping the base64 of a past transaction, but it might be more obvious that it's not going to be accurate then.

I've done some testing of this locally and really like this! I have found a few issues though:

@brittcyr
Copy link
Contributor Author

Hey @brittcyr, thanks for contributing this, this is a great idea to add!

I do have one general concern/question about this approach, but I'm not sure there's a better alternative. In general for past transactions, the current balance isn't going to be an accurate pre-transaction balance. Do you have any thoughts about the impact of that?

I think this is going to be much more useful for a proposed transaction than for inspecting a past transaction. Do you think it'd make sense to disable this for the /tx/<id>/inspect page, and only use it when inspecting a transaction from base64? I'm curious what you're finding this most useful for though! And obviously nothing stops someone dumping the base64 of a past transaction, but it might be more obvious that it's not going to be accurate then.

I've done some testing of this locally and really like this! I have found a few issues though:

I am not targeting past transactions. I am not actually aware of anyone using the simulate other than linking from realms proposal which is the use case I am desiring. Disabling it on past tx inspect seems right. Would be a lot of work and outside the scope of this to get the pre-balance for that use case.

Other comments seem doable, will address them.

@vercel
Copy link

vercel bot commented Aug 31, 2023

@brittcyr is attempting to deploy a commit to the Solana Labs Team on Vercel.

A member of the Team first needs to authorize it.

@brittcyr
Copy link
Contributor Author

Not sure what to do about the address lookup tables since I need getAccountKeys before simulation. Should I just skip the token balance changes in that case?

@mcintyre94
Copy link
Collaborator

I am not targeting past transactions. I am not actually aware of anyone using the simulate other than linking from realms proposal which is the use case I am desiring.

Got you, makes a lot of sense for a proposal for sure! I think just hide it when we know it's a past transaction, or when simulation fails. This should work really nicely then!

Not sure what to do about the address lookup tables since I need getAccountKeys before simulation. Should I just skip the token balance changes in that case?

I'm not really familiar with this web3js API TBH. But can you use message.resolveAddressLookupTables? https://solana-labs.github.io/solana-web3.js/classes/MessageV0.html#resolveAddressTableLookups Might need to look more into this though!

@brittcyr
Copy link
Contributor Author

  • T3Bo3gmGMkpmPUEVKiQ7CWqU6HFNPce63TaPe8k59aDYa9p376VkyKjej1QCHE49m8h9erbhzeZtqX1171Wnw1K

do you have an example of address table lookups on a base64 inspector tx?

@mcintyre94
Copy link
Collaborator

You can use that transaction as base64 on mainnet:

ARZ0M+KuIaQSPS6j1Kf4KfPLVV/Xu7Dl1CCThwPzUdpkYC6EGcTkKKc8CTatRgqYcUkbPAstWzU47Xr7yL/ZowKAAQAHC3maBMLkV5LzuqX/7ToBxoQwBelW83WQqHLXCeu5fYQFCKtORSAAGe3iGWbVj87K0JVp8eHr1TqLwMywg6BcjtwKBSRBoVhOV4xwBfanvh4FdBl3V3APJBkA8l9K/YoZzg1kTL4ts5oE1wHY1u+a8zoDushoiLFircTVAC+fC6vwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAADBkZv5SEXMv/srbpyw5vnvIzlu8X3EmssQ5s6QAAAAAR51VvyMcBu7nTFbs5oFQf9sbLeo/SOUQKxzaJWvBOPBt324ddloZPZy+FGzut5rBy0he1fWzeROoz1hX7/AKmMlyWPTiSJ8bs9ECkUjg2DC1oTmdr/EIQEjnvY2+n4WbQ/+if11/ZKdMCbHylYed5LCas238ndUUsyGqezjOXo4vh36D2uxoiPQ6u1rnsxG7eMNJYIl/zlp7gsFuIP8Ze+60iy4dQ9F7P+H/r1Zn6taV0QqzcndA08lQfcZuuSNAQFAAUCwFwVAAgGAAEADwQHAQEGHAcKAAMDAgERDwYQCQYTEhQAAw4NAgsMEQ8QBxAkwSCbM0HWnIEAAQAAAB5kAAEkBwhFAAAAACENnBAAAAAAMgAABwMBAAABCQH9QKy7pCQfVqHZ8aPpnAIxlPWQsH0NTGkkpgg1woyRXgTGx8XEBjWuyMLBww==

@vercel
Copy link

vercel bot commented Sep 1, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
explorer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 14, 2023 9:43am

@brittcyr
Copy link
Contributor Author

brittcyr commented Sep 4, 2023

Handled address lookup tables and made the token balances not show up with an error.

@brittcyr brittcyr requested a review from mcintyre94 September 4, 2023 19:13
@DonDuala
Copy link

DonDuala commented Sep 8, 2023

Bump @mcintyre94

Copy link
Collaborator

@mcintyre94 mcintyre94 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay @brittcyr and thanks for the bump @DonDuala! This is really close now :)

I've made a suggestion where I think we need to use a ternary

In addition I'm seeing a slight styling issue at the bottom of the Token Balances card - do you know where that might be coming from?
Screenshot 2023-09-13 at 16 49 09

app/components/inspector/SimulatorCard.tsx Outdated Show resolved Hide resolved
@brittcyr
Copy link
Contributor Author

Sorry for the delay @brittcyr and thanks for the bump @DonDuala! This is really close now :)

I've made a suggestion where I think we need to use a ternary

In addition I'm seeing a slight styling issue at the bottom of the Token Balances card - do you know where that might be coming from? Screenshot 2023-09-13 at 16 49 09

Issue was a card inside a card. Fixed

@brittcyr brittcyr requested a review from mcintyre94 September 13, 2023 18:34
@mcintyre94
Copy link
Collaborator

This looks great! Thanks for the contribution and for your patience, @brittcyr!

@mcintyre94 mcintyre94 merged commit b91b274 into solana-labs:master Sep 14, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants