Implement getTFTBillingRate #4430
Conversation
… w getTFTBillingRate in grid client & playground
zaelgohary
requested review from
0oM4R,
AhmedHanafy725,
Mahmoud-Emad,
MohamedElmdary and
amiraabouhadid
as code owners
| const minPrice = (minRes.toPrimitive() as number) ?? 0; | ||
| const maxPrice = (maxRes.toPrimitive() as number) ?? 0; | ||
| const averagePrice = (avgRes.toPrimitive() as number) ?? 0; |
There was a problem hiding this comment.
I’m not deep into TypeScript, but this raises a question: is there a reason we’re masking the failure rather than throwing?
I think continuing with a missing or invalid value is risky, as it will produce incorrect billing while giving no indication that something went wrong, and it completely obscures the real issue, making debugging much harder.
| private async convertToTFT(USD: Decimal) { | ||
| try { | ||
| const tftPrice = (await this.client.tftPrice.get()) ?? 0; | ||
| const tftPrice = (await this.client.tftPrice.getTFTBillingRate()) ?? 0; |
There was a problem hiding this comment.
The same error masking happens here
| return priceInMili / 1000; | ||
| } | ||
|
|
||
| async getTFTBillingRate(): Promise<number> { |
There was a problem hiding this comment.
The same name getTFTBillingRate() is used at the tfchain_client and tfgrid_client, but they return different units. In the call sites, some divide by 1000 after getting the value, some don't, based on the place/client where they import the method. For example, Callers using the grid client should not divide again.
I would suggest changing the tfgrid client method name to getTFTBillingRateUSD() to avoid potential confusion about the unit returned at call sites. Optionally, the tfchain client method name can also reflect the unit (getTFTBillingRateMUSD)
| } | ||
|
|
||
| @checkConnection | ||
| async getTFTBillingRateMUSD(): Promise<number> { |
There was a problem hiding this comment.
The uppercase M sounds like it's millions not milli
| async getTFTBillingRateMUSD(): Promise<number> { | |
| async getTFTBillingRatemUSD(): Promise<number> { | |
There was a problem hiding this comment.
The capital 'M' in MUSD was used strictly to adhere to camel case convention. Within this repo, we exclusively deal with standard USD and milli-USD; we do not use the Mega prefix anywhere. Therefore, the limited scope of our units ensures there is no ambiguity for the reader.
#4340