Skip to content

feat: adding TRS balance property to wallet#2252

Closed
MaxVTurcan wants to merge 1 commit into
masterfrom
wallet_trs_balance
Closed

feat: adding TRS balance property to wallet#2252
MaxVTurcan wants to merge 1 commit into
masterfrom
wallet_trs_balance

Conversation

@MaxVTurcan

Copy link
Copy Markdown

This property is required for reporting features

CLOSES Ticket: BG-45329

This property is required for reporting features

CLOSES Ticket: BG-45329
@codecov

codecov Bot commented May 4, 2022

Copy link
Copy Markdown

Codecov Report

Merging #2252 (5435afb) into master (3c1ba89) will increase coverage by 52.61%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #2252       +/-   ##
===========================================
+ Coverage        0   52.61%   +52.61%     
===========================================
  Files           0        6        +6     
  Lines           0      574      +574     
  Branches        0       94       +94     
===========================================
+ Hits            0      302      +302     
- Misses          0      262      +262     
- Partials        0       10       +10     
Flag Coverage Δ
unit 52.61% <ø> (?)

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

Impacted Files Coverage Δ
modules/express/src/expressApp.ts 89.51% <0.00%> (ø)
modules/express/src/retryPromise.ts 11.76% <0.00%> (ø)
modules/express/src/clientRoutes.ts 32.48% <0.00%> (ø)
modules/express/src/config.ts 94.73% <0.00%> (ø)
modules/express/src/args.ts 100.00% <0.00%> (ø)
modules/express/src/errors.ts 82.35% <0.00%> (ø)

*
* This is useful when balances have the potential to overflow standard javascript numbers
*/
trsBalanceString(): string {

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.

I'm not sure how this solves the problem in the ticket. Probably missing some context. Can you expand on how these new fields are utilized with an example?

@MaxVTurcan MaxVTurcan May 5, 2022

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.

Sure, these wallet objects are passed back to bga when executing the wallet command and then used to display the result:
https://github.com/BitGo/bitgo-admin/blob/master/src/handlers/wallet.js#L151

I wanted to add the property to the object instead of doing a direct property reference on a undeclared field. The TRS balance field will go right below the other 3 balances that get printed.

This PR will work in tandem with 2 other PRs to achieve this. Here is the PR that will populate that new field:
https://github.com/BitGo/bitgo-microservices/pull/18313/files#diff-008ef6124f852724918acc1cb7c461bd3409c7afae62c2740b9952e00f81fd94R417

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.

To make sure, these are fields that only for bga?

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.

I'm in agreement with @tylerlevine's latest comment. Since this is internal only on an open source project, the code should be implemented internally per Tyler's comment.

@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.

This should not be part of the public facing API - if we want to get a balance from TRS, it should be optional (and opted into via a query param or request body option) and should replace the balanceString.

We can't expect customers to know what trs is, and we should not be exposing internal service names through the public API in this way. I'll leave more comments on the wallet platform change.

@MaxVTurcan

Copy link
Copy Markdown
Author

@MaxVTurcan

MaxVTurcan commented May 31, 2022

Copy link
Copy Markdown
Author

This should not be part of the public facing API - if we want to get a balance from TRS, it should be optional (and opted into via a query param or request body option) and should replace the balanceString.

We can't expect customers to know what trs is, and we should not be exposing internal service names through the public API in this way. I'll leave more comments on the wallet platform change.

Got it @tylerlevine, what you are saying makes sense. I will work on making updates and see what I come up with. I need to clarify if it is ok to replace balanceString completely in the bga output.

@MaxVTurcan

Copy link
Copy Markdown
Author

No longer needed

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.

3 participants