solana icon indicating copy to clipboard operation
solana copied to clipboard

feat: overload getTransaction to support versioned transactions

Open jstarry opened this issue 1 year ago • 1 comments

Problem

Connection.getTransaction supports the maxSupportedTransactionVersion config but its return type contains a Message class which doesn't support versioned messages.

Summary of Changes

  • Overload Connection.getTransaction with a new method whose return value allows devs to migrate to handling versioned transactions
  • Deprecate the current Connection.getTransaction method
  • [ ] Needs tests

Fixes #

jstarry avatar Aug 10 '22 21:08 jstarry

Codecov Report

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

:exclamation: 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     

codecov[bot] avatar Aug 10 '22 21:08 codecov[bot]

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 e9b08b5e7fb5022072e6331c5c15aceda8d8fd3c over a year ago. Please discuss if you have any concerns about this, thanks!

jstarry avatar Aug 25 '22 18:08 jstarry

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.

steveluscher avatar Aug 30 '22 06:08 steveluscher

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 avatar Aug 30 '22 17:08 jstarry