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

feat(balance-info): add query param to convert free balance to human #914

Merged
merged 20 commits into from
May 16, 2022

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented May 6, 2022

Feature: denominated queryParam for /accounts/{accountId}/balance-info

Summary: Returns the atomic balance as a human readable number based on the given chains token decimal. Currently the query param is a boolean so you would just pass in ?denominated=true.

@TarikGul TarikGul requested a review from a team as a code owner May 6, 2022 19:39
): Promise<IAccountBalanceInfo> {
const { api } = this;
const decimal = api.registry.chainDecimals[0];
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we know that this value will always be there for a substrate based chain, or is it safe to say we should check and throw an error if it doesnt exist?

Copy link
Member

Choose a reason for hiding this comment

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

if a substrate base chain is using a SS58 address it will always exist AFAIU the list of addresses can be found https://github.com/paritytech/ss58-registry/blob/main/ss58-registry.json

however it's possible for a substrate based chain to use some other format.

Copy link
Member Author

@TarikGul TarikGul May 7, 2022

Choose a reason for hiding this comment

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

https://github.com/paritytech/ss58-registry/blob/main/ss58-registry.json

This is extremely helpful! Thanks so much. Yea based on seeing some of them having no decimal value within the array, I am going to throw an error for those who use the query param against a chain that doesnt have the value available. One of them in the list for example: subsocial.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(I wonder why the symbol and decimals are in an array; it sortof implies that there could be more than 1, too, in some cases?)

Copy link
Member Author

@TarikGul TarikGul May 9, 2022

Choose a reason for hiding this comment

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

Yea you are correct. After doing some research though, the order of the values in the array correspond to the values in the tokens array: So... If the available tokens in the chain are ['MOVR', 'GMBR'] -> then the chainsDecimals array would look like [18, 18]. I haven't seen a single case where the decimals change, BUT doesnt mean it cant. I should probably add a check for this.

@TarikGul
Copy link
Member Author

TarikGul commented May 6, 2022

Ill add the docs once we are happy with the naming, and what the goal of this query param should be.

Comment on lines +219 to +220
// If the denominated value will be less then zero, pad it correctly
if (strBalance.length <= dec) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm struggling to parse exactly how thse logic for this is working, but I can see that it's being tested so it's not a big deal, just curious really!

Copy link
Member Author

@TarikGul TarikGul May 9, 2022

Choose a reason for hiding this comment

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

Well the idea was how can we properly denominate the decimal places without ever having the string (number represented as a string) be converted to a number. This is because the atomic value given can be too large for javascript to interpret. In the logic above I wanted to make sure we also never have to use BN, or convert to a bigInt. We can easily fix the value to a decimal with some simple math, but because the values might be to big thats not possible.

So basically what the logic does takes care of three cases.

  1. If the value is zero just return zero

  2. If the decimal value is larger than the length of the atomic value (it's a string), the value denominated is going to be less than zero. Which means it needs the correct amount of padding (zeros) after the decimal.

  3. The value is greater than zero, and therefore handle the decimal places by subtracting the length of the value, by the decimal places, and using that difference to determine where we put the decimal.

Copy link
Member

@niklasad1 niklasad1 May 13, 2022

Choose a reason for hiding this comment

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

If the decimal value is larger than the length of the atomic value (it's a string), the value denominated is going to be less than zero. Which means it needs the correct amount of padding (zeros) after the decimal.

I don't follow that but maybe I'm just slow such you might have a number like 123.9999 (decimal is bigger than the number) why do you need to pad it?

ah, right the decimals is just some number such as 10 that the number should be multiplied with not an actual decimal point. Sorry I got confused by dec.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also had non-integers in mind, but makes sense to me after both of your comments; thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, right the decimals is just some number such as 10 that the number should be multiplied with not an actual decimal point. Sorry I got confused by dec.

Yup exactly!

Important to note as well, that the balance passed in will always be represented as an atomic balance via the Balance type.

@TarikGul TarikGul merged commit f1e03d6 into master May 16, 2022
@TarikGul TarikGul deleted the tarik-accounts-token-unit branch May 16, 2022 13:31
This pull request was closed.
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