-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
web3.js: Implement addSignature in VersionedTransaction #27945
Conversation
Codecov Report
@@ 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 |
There was a problem hiding this 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.
Pull request has been modified.
@steveluscher Pushed the updated error for invalid signers, added the check for signature length and a test. |
web3.js/test/transaction.test.ts
Outdated
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`, | ||
); |
There was a problem hiding this comment.
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', () => { /* ... */ })
});
Pull request has been modified.
@steveluscher I created a |
Thank you! |
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)