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

Fix Asset toAtomicAmount to not return scientific notation #250

Merged
merged 5 commits into from
Sep 18, 2024

Conversation

rohan-agarwal-coinbase
Copy link
Contributor

@rohan-agarwal-coinbase rohan-agarwal-coinbase commented Sep 18, 2024

What changed? Why?

Earlier, toAtomicAmount(new Decimal(1000)) where decimals was 18 would result in 2e+21, which then got serialized as a string and failed on our backend as an invalid_amount. This PR changes toAtomicAmount to return a bigint which gets serialized correctly.

Testing:

  • Unit tests added
  • Confirmed the unit test in wallet_address_test fails in v0.6.0 branch but succeeds with the rest of the changes in this PR
  • Tested end to end in prod with this command successfully:
const transfer = addr.createTransfer({amount: 1000, assetId: "0xfa32823cf44fCE33131312e973C72362CE1D9C55", destination: '0x339c1ca0c2bB9522D4d61C6562B2d49021f9a0C0'});

Qualified Impact

We're still returning a numeric but this is technically backward-incompatible. We expect external usages of this function to be low and it to be primarily used by our other classes.

src/tests/asset_test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@alex-stone alex-stone left a comment

Choose a reason for hiding this comment

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

Can we add a createTransfer test with a sufficiently large value that tests it gets passed properly (a test that would fail without this logic change)?

@rohan-agarwal-coinbase rohan-agarwal-coinbase merged commit 3e35577 into v0.6.0 Sep 18, 2024
6 checks passed
@rohan-agarwal-coinbase rohan-agarwal-coinbase deleted the rohan/fix-atomic-calculation branch September 18, 2024 22:05
howard-at-cb added a commit that referenced this pull request Sep 25, 2024
)

* add eventTypeFilter

* Update types.ts

* Update webhook_test.ts

* Update webhook.ts

* Update webhook_test.ts

* Update webhook.ts

* Update webhook_test.ts

* Update webhook_test.ts

* Update webhook_test.ts

* Update webhook_test.ts

* Update webhook_test.ts

* wallet.create_webhook

* Changelog

* Update wallet.ts

* Update wallet.ts

* Update wallet_test.ts

* Update wallet_test.ts

* Update wallet_test.ts

* Update wallet_test.ts

* Update wallet_test.ts

* fmt

* Update wallet_test.ts

* Update wallet_test.ts

* Update webhook.ts

* Update wallet_test.ts

* Update wallet_test.ts

* address comments

* fix test

* Update wallet_test.ts

* Update wallet_test.ts

* Update wallet_test.ts

* Update wallet_test.ts

* Update jest.config.js

* add sol, lamport asset suppport

* Fix Asset toAtomicAmount to not return scientific notation (#250)

* Fix merge conflicts between master and v0.6.0 (#255)

* fix arb

* update version

* remove network check logic from sdk

* fix coverage

* fix lint

* [PSDK-443] Quickstart Basenames Registration (#243)

* base names quickstart

* [PSDK-443] Quickstart Register Basenames for MPC Wallet

* [hotfix] Fix L2 Resolver Address on Register Basenames Quickstart (#244)

---------

Co-authored-by: Jayasudha Jayakumaran <jayasudha.jayakumaran@coinbase.com>
Co-authored-by: Jayasudha Jayakumaran <121061531+jazz-cb@users.noreply.github.com>
Co-authored-by: John Peterson <98187317+John-peterson-coinbase@users.noreply.github.com>

* address comments

* Update api.ts

* Update CHANGELOG.md

* apply new changes

* fix tests

* Update wallet.ts

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update jest.config.js

---------

Co-authored-by: Rohan Agarwal <rohan.agarwal.1@coinbase.com>
Co-authored-by: Rohit Durvasula <rohit.durvasula@coinbase.com>
Co-authored-by: Jayasudha Jayakumaran <jayasudha.jayakumaran@coinbase.com>
Co-authored-by: Jayasudha Jayakumaran <121061531+jazz-cb@users.noreply.github.com>
Co-authored-by: John Peterson <98187317+John-peterson-coinbase@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants