Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

RPC: getTokenAccountBalance/getTokenSupply should normalize the amount based on the mint's decimals value #11366

Closed
mvines opened this issue Aug 4, 2020 · 7 comments · Fixed by #11370
Assignees

Comments

@mvines
Copy link
Contributor

mvines commented Aug 4, 2020

In order to correctly represent a spl-token balance to the user, it needs to be normalized with the mint's decimals value. But getTokenAccountBalance/getTokenSupply don't provide this information. The native getBalance/getSupply methods don't suffer from this problem because the decimals are well known (9).

Suggested changes:

  1. getTokenAccountBalance/getTokenSupply by default return a f64 number normalized with the mint's decimals
  2. An optional input parameter to getTokenAccountBalance/getTokenSupply to permit the user to request the raw u64 amount.
@mvines
Copy link
Contributor Author

mvines commented Aug 4, 2020

The downstream affect of not having this facility can be seen here:
https://github.com/solana-labs/solana-program-library/blob/77d416ed7d7c08179f7485af75c21c756955d4e1/token-cli/src/main.rs#L66-L85

Inlined:

fn get_decimals_for_token(config: &Config, token: &Pubkey) -> Result<u8, Error> {
    if *token == native_mint::id() {
        Ok(native_mint::DECIMALS)
    } else {
        let mint = config
            .rpc_client
            .get_token_mint_with_commitment(token, config.commitment_config)?
            .value
            .ok_or_else(|| format!("Invalid token: {}", token))?;
        Ok(mint.decimals)
    }
}

fn ui_amount_to_amount(ui_amount: f64, decimals: u8) -> u64 {
    (ui_amount * 10_usize.pow(decimals as u32) as f64) as u64
}

fn amount_to_ui_amount(amount: u64, decimals: u8) -> f64 {
    amount as f64 / 10_usize.pow(decimals as u32) as f64
}

@mvines
Copy link
Contributor Author

mvines commented Aug 4, 2020

@jstarry - I suspect you'll rapidly run into this in the explorer UI as well.

@CriesofCarrots CriesofCarrots self-assigned this Aug 4, 2020
@CriesofCarrots
Copy link
Contributor

CriesofCarrots commented Aug 4, 2020

@mvines I have (1) ready to go. How important is (2) to you? It's just a slightly longer pull because we need to handle different return types. Worth blocking on?

@mvines
Copy link
Contributor Author

mvines commented Aug 4, 2020

2 seemed nice for completeness but I don't have an immediate need for it. 👢

@jstarry
Copy link
Contributor

jstarry commented Aug 5, 2020

It seems like we have two use cases here:

  1. Display token balance
  2. Transact in tokens

For (1) this change is nice, but it could result in loss of precision
For (2) this change is bad, apps will need to convert a float to a decimal with no loss of precision.

The current API doesn't support (2) well either because there will be loss of precision for large integers. I think we should always return the raw amount (as a string) in addition to the float

@mvines
Copy link
Contributor Author

mvines commented Aug 5, 2020

Nice idea, I like it

@jstarry
Copy link
Contributor

jstarry commented Aug 5, 2020

We also need to do a full audit of all of our integer use in JSON APIs soon 😉

@mergify mergify bot closed this as completed in #11370 Aug 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants