solana-labs / solana

Web-Scale Blockchain for fast, secure, scalable, decentralized apps and marketplaces.
https://solanalabs.com
Apache License 2.0
13.02k stars 4.19k forks source link

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

Closed mvines closed 4 years ago

mvines commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

CriesofCarrots commented 4 years ago

@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 commented 4 years ago

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

jstarry commented 4 years ago

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 commented 4 years ago

Nice idea, I like it

jstarry commented 4 years ago

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