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

feat: update Connection to support versioned transactions #27068

Merged
merged 1 commit into from
Aug 31, 2022

Conversation

jstarry
Copy link
Member

@jstarry jstarry commented Aug 10, 2022

Problem

Methods like Connection.getTransaction and Connection.getBlock support the maxSupportedTransactionVersion config but the return type contains a Message class which doesn't support versioned messages.

Summary of Changes

  • Deprecate the usage of getTransaction, getTransactions, and getBlock RPC APIs without passing a maxSupportedTransactionVersion in the config
  • Add support for versioned transaction fields in getTransaction, getParsedTransaction, getBlock, and getTransactions responses

This change is not a breaking change because the responses for the modified Connection methods only include a versioned message when a user opts into receiving versioned transactions with the maxSupportedTransactionVersion config.

Fixes #

@jstarry jstarry added the work in progress This isn't quite right yet label Aug 10, 2022
@codecov
Copy link

codecov bot commented Aug 10, 2022

Codecov Report

Merging #27068 (df71894) into master (ada493f) will decrease coverage by 0.4%.
The diff coverage is n/a.

❗ Current head df71894 differs from pull request most recent head 310e7a1. Consider uploading reports for the commit 310e7a1 to get more accurate results

@@            Coverage Diff            @@
##           master   #27068     +/-   ##
=========================================
- Coverage    76.7%    76.2%   -0.5%     
=========================================
  Files          52       52             
  Lines        2651     2674     +23     
  Branches      364      372      +8     
=========================================
+ Hits         2035     2040      +5     
- Misses        483      495     +12     
- Partials      133      139      +6     

@jstarry jstarry changed the title feat: overload getTransaction to support versioned transactions feat: update getTransaction to support versioned transactions Aug 22, 2022
@jstarry jstarry removed the work in progress This isn't quite right yet label Aug 25, 2022
@jstarry
Copy link
Member Author

jstarry commented Aug 25, 2022

There is a breaking change in this update so according to semver we should bump the major version of the package. Last time we did this was in e9b08b5 over a year ago. Please discuss if you have any concerns about this, thanks!

@jstarry jstarry changed the title feat: update getTransaction to support versioned transactions feat: update Connection to support versioned transactions Aug 29, 2022
@jstarry jstarry changed the title feat: update Connection to support versioned transactions feat!: update Connection to support versioned transactions Aug 29, 2022
@steveluscher
Copy link
Contributor

There are a few strategies to roll a change to return types incrementally that we could use here.

1. Rename, deprecate, then remove

We could deliver VersionedMessage against a property with a new name, like versionedMessage and then wrap the old property in an accessor that logs a message.

const ret = {};
Object.defineProperty(ret, 'message', {
  get() {
    console.warn('`TransactionResponse::message` is deprecated and will be removed in a future version. Use `TransactionResponse::versionedMessage` instead');
    return message;
  },
});
return ret;

2. Sidecar methods

Instead of changing the old methods, create new ones: getVersionedTransaction, getVersionedBlock, etc…


Either of these would let us continue to ship new unrelated features into web3.js without forcing folks to deal with this transaction versioning upgrade now. If we make this breaking change cold, then ship an unrelated bug fix, you'll have people on one side of the rubicon that can't get the bugfix until they do the versioned transaction refactor.

@jstarry
Copy link
Member Author

jstarry commented Aug 30, 2022

I opted for overloading the methods and only returning a versioned transaction type if maxSupportedTransactionVersion is set by the caller. That way we keep backwards compatibility and have a simple upgrade path which the deprecations guide developers through (and keep the familiar method names).

If we make this breaking change cold, then ship an unrelated bug fix, you'll have people on one side of the rubicon that can't get the bugfix until they do the versioned transaction refactor.

Yeah, that would be a headache. It'd be great to queue up have infra that allows us to ship to multiple major versions. Until then, it's not worth the risk and overhead of shipping a new major version right now

@jstarry jstarry changed the title feat!: update Connection to support versioned transactions feat: update Connection to support versioned transactions Aug 30, 2022
Copy link
Contributor

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

Phew! Thanks!

Didn't look at the code in detail, just approving the approach in principle. cc/ @jordansexton

Copy link
Contributor

@jordaaash jordaaash left a comment

Choose a reason for hiding this comment

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

I had the same concerns as @steveluscher around making this a breaking change at this time, but this approach looks solid!

@jstarry jstarry merged commit 292b2a1 into solana-labs:master Aug 31, 2022
@jstarry jstarry deleted the web3/get-transaction branch August 31, 2022 12:46
apfitzge pushed a commit to apfitzge/agave that referenced this pull request Aug 31, 2022
…abs#27068)

feat: update Connection to support versioned transactions
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.

3 participants