-
Notifications
You must be signed in to change notification settings - Fork 33
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 bigInt serialisation Issue #25
Fix bigInt serialisation Issue #25
Conversation
ENG-2524 Error calling sendBtcTransaction
Developer reported error when using the sendBtcTransaction function: |
src/addresses/types.ts
Outdated
@@ -1,19 +1,19 @@ | |||
import type { RequestOptions, RequestPayload } from '../types'; | |||
|
|||
export enum AddressPurpose { | |||
export enum AddressPurposes { |
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.
why are we changing these?
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.
The PR also fixes a bug where the AddressPurposes type name has been reverted to what it originally was since this attribute is used in extension and app.
@@ -20,7 +30,14 @@ export const sendBtcTransaction = async (options: SendBtcTransactionOptions) => | |||
} | |||
|
|||
try { | |||
const request = createUnsecuredToken(options.payload as unknown as Json); | |||
const serializedRecipients: SerializedRecipient[] = serializer(recipients); | |||
const serialisedPayload: SerializedSendBtcTransactionPayload = { |
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.
should be serializedPayload
@@ -11,7 +21,7 @@ export const sendBtcTransaction = async (options: SendBtcTransactionOptions) => | |||
throw new Error('No Bitcoin wallet installed'); | |||
} | |||
|
|||
const { recipients, senderAddress } = options.payload; | |||
const { recipients, senderAddress, network, message } = options.payload; | |||
if (!recipients || recipients.length === 0) { |
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.
we need to also add a check on the recipients's type before attempting to serialize it and pass it to the extension
|
* added signMessage method to provider * throw error on missing options * Include send btc transaction function * Remove console.log * convert to amountSats * make message optional * Allow to pass a custom Bitcoin provider (#14) * Allow to pass a custom Bitcoin provider * Extract GetBitcoinProviderFunc type * Add senderAddress and allow multiple recipients (#10) * Add senderAddress and allow multiple recipients * Use Recipient in payload * Use bigint type for amountSats * Improve readability and error messages (#15) * Improve readability * Clean up error messages * Remove duplicate line * Improve error message --------- Co-authored-by: Victor Kirov <victor.kirov@gmail.com> * Add NPM pull request preview publish github workflow (#22) * Add NPM pull request preview publish github workflow * Add NPM pull request preview publish github workflow * fix build script * add npmrc file * add npm token * add npmrc file * change package repository to npm * update release workflow * release v0.5.0 (#23) * Improve APIs and types (BREAKING) (#16) * Remove unused type * Rename AddressPurposes enum for best practices * Extract call types into separate file * Remove unused CallMethod enum * Extract provider types into separate file * Rename directories for consistency * Rename sendBtcTransaction -> sendTransaction * Extract transactions types to separate file * Extract networks module * Improve types * Use [] instead of Array<> for consistency * Fix copy-paste oversight * Move index.ts inside src * Use import type for types * Revert sendBtcTransaction -> sendTransaction renaming * Simplify types --------- Co-authored-by: Victor Kirov <victor.kirov@gmail.com> * Fix bigInt serialisation Issue (#25) * serialize recipient array * Fix AddressPurposes type * Revert AddressPurpose name change * Rename const * Check recipient type * chore: npm publish with prerelease tag if prerelease option selected (#27) * chore: update release-package.yml syntax (#28) * Release v1.0.0 (#24) release v1.0.0 Co-authored-by: Tim Man <tim@secretkeylabs.com> --------- Co-authored-by: Ken Liao <yukanliao@gmail.com> Co-authored-by: Imamah-Zafar <imamah.zafar@tintash.com> Co-authored-by: Duska.T <55587184+DuskaT021@users.noreply.github.com> Co-authored-by: Matías Olivera <moliverafreire@gmail.com> Co-authored-by: Imamah-Zafar <88320460+Imamah-Zafar@users.noreply.github.com> Co-authored-by: Victor Kirov <victor.kirov@gmail.com> Co-authored-by: Tim Man <tim@secretkeylabs.com>
The type of amountSats is set as bigint but when the bigint value is set for amountSats in recipient array in sats connect example app, an error is thrown
Donot know how to serialize a BigInt
.The reason for this is BigInt value will raise a TypeError if called in
JSON.stringify
, as BigInt values aren't serialized .For more context, see the official documentation: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt#operators
The
JSON.stringify
is called in functioncreateUnsecuredToken
and throws the error.The PR fixes this issue by serialising the recipient array so that the payload can be sent to
createUnsecuredToken
and then further in the extension and app.The PR also adds type validation for recipient array
For testing
Extension PR: secretkeylabs/xverse-web-extension#543
secretkeylabs/xverse-web-extension#546
Sats Connect Example PR: secretkeylabs/sats-connect-example#8
Test the send BTC function with different values for the recipient array by changing the amountSats value to number, decimal and then bigint. It should only work for bigint.
Also play around with the recipient type by changing from string to see if the error
Incorrect recipient format
comes up.You can make changes to the values of address and amountSats by making changes in the recipient object on line https://github.com/secretkeylabs/sats-connect-example/blob/3ff09d43b7e9523d1552ed2d12b125ac918b15d5/src/dashboard.js#L197 in the test app