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

web3.js: Implement addSignature in VersionedTransaction #27945

Merged

Conversation

vsakos
Copy link
Contributor

@vsakos vsakos commented Sep 20, 2022

Problem

Unlike the legacy Transaction class, the new VersionedTransaction class has no addSignature() function to add signatures that were generated externally (by a third party library, browser extension etc.).

Summary of Changes

Implemented addSignature(publicKey: PublicKey, signature: Uint8Array)

@mergify mergify bot added the community Community contribution label Sep 20, 2022
@mergify mergify bot requested a review from a team September 20, 2022 19:00
@vsakos vsakos changed the title Implement addSignature in VersionedTransaction web3.js Implement addSignature in VersionedTransaction Sep 20, 2022
@vsakos vsakos changed the title web3.js Implement addSignature in VersionedTransaction web3.js: Implement addSignature in VersionedTransaction Sep 20, 2022
@codecov
Copy link

codecov bot commented Sep 20, 2022

Codecov Report

Merging #27945 (7601207) into master (e6687b8) will increase coverage by 0.4%.
The diff coverage is 96.6%.

@@            Coverage Diff            @@
##           master   #27945     +/-   ##
=========================================
+ Coverage    77.3%    77.7%   +0.4%     
=========================================
  Files          55       55             
  Lines        2889     2916     +27     
  Branches      402      404      +2     
=========================================
+ Hits         2234     2267     +33     
+ Misses        514      509      -5     
+ Partials      141      140      -1     

@steveluscher steveluscher added the javascript Pull requests that update Javascript code label Sep 20, 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.

Thanks for this! Let's get some tests written to cover this functionality and the invariants you want to trip, and let's ship it – unless @jstarry has additional thoughts.

web3.js/src/transaction/versioned.ts Show resolved Hide resolved
web3.js/src/transaction/versioned.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed steveluscher’s stale review September 20, 2022 21:17

Pull request has been modified.

@mergify mergify bot requested a review from a team September 20, 2022 21:17
@vsakos
Copy link
Contributor Author

vsakos commented Sep 20, 2022

@steveluscher Pushed the updated error for invalid signers, added the check for signature length and a test.

steveluscher
steveluscher previously approved these changes Sep 20, 2022
Comment on lines 1001 to 1008
expect(() => {
transaction.addSignature(signer.publicKey, new Uint8Array(32));
}).to.throw('Signature must be 64 bytes long');
expect(() => {
transaction.addSignature(invalidSigner.publicKey, new Uint8Array(64));
}).to.throw(
`Can not add signature; \`${invalidSigner.publicKey.toBase58()}\` is not required to sign this transaction`,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

For sure want these to be separate tests, so that when they fail, the reader doesn't have to dig to find out what functionality was broken.

describe('addSignature', () => {
  it('appends an externally generated signature at the correct index', () => { /* ... */ })
  it('fatals when the signature is the wrong length', () => { /* ... */ })
  it('fatals when adding a signature for a public key that has not been marked as a signer', () => { /* ... */ })
});

web3.js/test/transaction.test.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed steveluscher’s stale review September 20, 2022 21:54

Pull request has been modified.

@mergify mergify bot requested a review from a team September 20, 2022 21:54
@vsakos
Copy link
Contributor Author

vsakos commented Sep 20, 2022

@steveluscher I created a VersionedTransaction group with separate tests for addSignature (using the same transaction instance). What do you think of this?

@steveluscher steveluscher merged commit e15a5c0 into solana-labs:master Sep 22, 2022
@steveluscher
Copy link
Contributor

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants