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

fix: allow mutating transactions #25141

Merged
merged 1 commit into from
May 12, 2022
Merged

Conversation

jstarry
Copy link
Member

@jstarry jstarry commented May 11, 2022

Problem

After deserializing a transaction, it's not possible to sign and send because sending the transaction could mutate the transaction's blockhash and cause a mutation error.

Summary of Changes

Allow mutating the recent blockhash of a deserialized transaction

Fixes #25137

@jstarry jstarry requested a review from jordaaash May 11, 2022 17:22
@codecov
Copy link

codecov bot commented May 11, 2022

Codecov Report

Merging #25141 (d70c81b) into master (05de0e3) will decrease coverage by 6.9%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #25141       +/-   ##
===========================================
- Coverage    82.0%    75.1%     -7.0%     
===========================================
  Files         610       38      -572     
  Lines      167945     2243   -165702     
  Branches        0      323      +323     
===========================================
- Hits       137868     1686   -136182     
+ Misses      30077      444    -29633     
- Partials        0      113      +113     

@jordaaash
Copy link
Contributor

Hmm, by the same logic, I could see an argument for allowing the fee payer, or even instructions, to be mutated, as long as the transaction hasn't been signed yet. Maybe the change that should be made is not to remove these fields but to make the check specific to signed transactions?

@jstarry
Copy link
Member Author

jstarry commented May 12, 2022

Hmm, by the same logic, I could see an argument for allowing the fee payer, or even instructions, to be mutated, as long as the transaction hasn't been signed yet. Maybe the change that should be made is not to remove these fields but to make the check specific to signed transactions?

Good point. What do you think about allowing anything in the transaction to be modified? We just trust devs to know that if they modify the transaction, any existing signatures would be invalidated. The main thing that #23720 fixed was ensuring that a deserialized transaction would serialize to the same thing if it wasn't touched.

@jstarry jstarry changed the title fix: allow mutating the blockhash in a deserialized tx fix: allow mutating transactions May 12, 2022
@jordaaash
Copy link
Contributor

We just trust devs

So crazy it just might work!

But yeah, throwing an error isn't great either, so maybe we just reserialize if changed?

@jstarry
Copy link
Member Author

jstarry commented May 12, 2022

But yeah, throwing an error isn't great either, so maybe we just reserialize if changed?

Yeah, that's exactly what I'm thinking. Let me know what you think of the updated PR

@jordaaash
Copy link
Contributor

LGTM!

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.

Partial Sign Transaction jupiter api
2 participants