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

Simulate: Don't attempt to sign transaction #2331

Merged
merged 3 commits into from Jan 17, 2023

Conversation

ckamm
Copy link
Contributor

@ckamm ckamm commented Dec 21, 2022

It can be very useful to simulate a transaction and retrieve logs / account state without bothering the user about it.

@vercel
Copy link

vercel bot commented Dec 21, 2022

@ckamm is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

@Henry-E
Copy link
Contributor

Henry-E commented Dec 22, 2022

For some reason it seems wrong to me that you would never want the wallet to sign a simulated transaction. If only for the purposes of testing and getting a failed transaction from the validator.

Can you just explain a little more the reasoning behind making it the default to never attach a signature vs. surfacing sigVerify: false as a top level argument (while also respecting the sigverify option and omitting the request for a wallet signature).

There must be some downside to removing the wallet signature from transaction simulate?

@ckamm
Copy link
Contributor Author

ckamm commented Dec 26, 2022

@Henry-E Exposing the option sounds right to me. There may indeed be cases where you'd want to test the signatures while simulating.

@ckamm ckamm force-pushed the ckamm/simulate-without-sign branch from 0085d00 to 8b6f2ee Compare December 26, 2022 15:15
@ckamm
Copy link
Contributor Author

ckamm commented Dec 26, 2022

How about this -- sign and set sigVerify: true only if the signers arg is set.

@Henry-E
Copy link
Contributor

Henry-E commented Dec 26, 2022

Sure, that could work. It would be similar to how it works here in this pr where the fee payer is set automatically unless it has already been set.

The only possible issue with this though is that maybe the transaction construction functions add signers automatically. Though tbh, I think it's probably reasonable to just let people get rid of the signers themselves.

The other minor issue with this is that it's hidden functionality, someone might not figure out this is how the function works without looking at the source or accidentally deleting the signers.

Overall though, I think this is seems like an ok way to go. Just so long as we add it clearly to the ts doc string that if there are no signers signer, the sigverify: false option will still allow for the function to simulated.

@ckamm ckamm force-pushed the ckamm/simulate-without-sign branch from 8b6f2ee to 9a4d8e3 Compare December 26, 2022 15:44
@ckamm
Copy link
Contributor Author

ckamm commented Dec 26, 2022

Added note in docs

@Henry-E Henry-E merged commit e3fe24f into coral-xyz:master Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants