Skip to content

feat(statics): add fiatusd and tfiatusd coins#2261

Merged
tylerlevine merged 1 commit into
masterfrom
STLX-13382_create_fiatusd_coin
May 13, 2022
Merged

feat(statics): add fiatusd and tfiatusd coins#2261
tylerlevine merged 1 commit into
masterfrom
STLX-13382_create_fiatusd_coin

Conversation

@emiacc

@emiacc emiacc commented May 6, 2022

Copy link
Copy Markdown

Add fiatusd and tfiatusd coins to support fiat backing wallets creation.

STLX-13382

@emiacc emiacc force-pushed the STLX-13382_create_fiatusd_coin branch from df66e00 to 73b6c39 Compare May 6, 2022 15:08
@emiacc emiacc requested review from drew-bitgo and dszheng92 May 6, 2022 15:43
@emiacc emiacc force-pushed the STLX-13382_create_fiatusd_coin branch from 73b6c39 to c46dd7a Compare May 6, 2022 15:44
Comment thread modules/statics/test/unit/coins.ts Outdated
Comment thread modules/statics/test/unit/coins.ts Outdated
Comment thread modules/statics/src/coins.ts Outdated
Comment thread modules/statics/src/account.ts Outdated

@drew-bitgo drew-bitgo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All of my comments are strictly formatting, no functional changes. Otherwise I Approve. A few questions for the SDK team, as well.

@emiacc emiacc force-pushed the STLX-13382_create_fiatusd_coin branch 2 times, most recently from 808d709 to e78e65e Compare May 6, 2022 16:52
@emiacc emiacc changed the base branch from master to STLX-13382_remove_fiat_tokens May 6, 2022 16:53
@emiacc emiacc requested a review from tylerlevine May 6, 2022 17:39
@emiacc emiacc changed the base branch from STLX-13382_remove_fiat_tokens to master May 9, 2022 15:35
@emiacc emiacc marked this pull request as ready for review May 9, 2022 15:36
@emiacc emiacc requested review from a team and drew-bitgo May 9, 2022 15:36
drew-bitgo
drew-bitgo previously approved these changes May 9, 2022
@emiacc emiacc force-pushed the STLX-13382_create_fiatusd_coin branch from e78e65e to 7a6fd7f Compare May 9, 2022 16:42
@codecov

codecov Bot commented May 9, 2022

Copy link
Copy Markdown

Codecov Report

Merging #2261 (0d68381) into master (6951b40) will increase coverage by 86.13%.
The diff coverage is 100.00%.

❗ Current head 0d68381 differs from pull request most recent head 1750a43. Consider uploading reports for the commit 1750a43 to get more accurate results

@@             Coverage Diff             @@
##           master    #2261       +/-   ##
===========================================
+ Coverage        0   86.13%   +86.13%     
===========================================
  Files           0      198      +198     
  Lines           0     9686     +9686     
  Branches        0     1436     +1436     
===========================================
+ Hits            0     8343     +8343     
- Misses          0      919      +919     
- Partials        0      424      +424     
Flag Coverage Δ
unit 86.13% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
modules/statics/src/coins.ts 100.00% <ø> (ø)
modules/statics/src/networks.ts 100.00% <ø> (ø)
modules/statics/src/account.ts 79.01% <100.00%> (ø)
...ccount-lib/src/coin/dot/batchTransactionBuilder.ts 71.42% <0.00%> (ø)
modules/account-lib/src/coin/hbar/index.ts 100.00% <0.00%> (ø)
modules/account-lib/src/coin/avaxc/keyPair.ts 100.00% <0.00%> (ø)
modules/account-lib/src/coin/hbar/transaction.ts 92.75% <0.00%> (ø)
modules/account-lib/src/coin/hbar/utils.ts 87.00% <0.00%> (ø)
modules/account-lib/src/coin/sol/transaction.ts 95.10% <0.00%> (ø)
modules/account-lib/src/coin/xrp/keyPair.ts 100.00% <0.00%> (ø)
... and 191 more

@emiacc emiacc requested a review from mmcshinsky-bitgo May 10, 2022 13:03

@tylerlevine tylerlevine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

seems pretty reasonable. Debatably we should not reuse the account convenience function for creating fiat coin instances and should have a separate one. And maybe even a separate child class from BaseCoin altogether. What do you think?

Comment thread modules/bitgo/src/v2/coins/fiatusd.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Careful with the unsafe default here - can this throw MethodNotImplementedError() instead?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In this case I opt to return the default value since I'm emulating the same behavior as SUSD

@emiacc emiacc force-pushed the STLX-13382_create_fiatusd_coin branch from 7a6fd7f to 804ee94 Compare May 11, 2022 14:20
@emiacc

emiacc commented May 11, 2022

Copy link
Copy Markdown
Author

seems pretty reasonable. Debatably we should not reuse the account convenience function for creating fiat coin instances and should have a separate one. And maybe even a separate child class from BaseCoin altogether. What do you think?

Thanks @tylerlevine for your suggestions. I added the new child class and the new factory function to create the Fiat coins.

@emiacc emiacc requested a review from tylerlevine May 11, 2022 15:20
add fiatusd and tfiatusd coins to support fiat backing wallets

STLX-13382
@emiacc emiacc force-pushed the STLX-13382_create_fiatusd_coin branch from 804ee94 to 1750a43 Compare May 13, 2022 12:58

@tylerlevine tylerlevine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@tylerlevine tylerlevine merged commit 041c1fa into master May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants