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.

Sign up for GitHub

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

Jump to bottom

Return token amounts as floats #11370

Merged
merged 5 commits into from
Aug 5, 2020
Merged

Return token amounts as floats #11370

merged 5 commits into from
Aug 5, 2020

Conversation

CriesofCarrots
Copy link
Contributor

@CriesofCarrots CriesofCarrots commented Aug 4, 2020 •
edited
Loading

Problem

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.

Summary of Changes

  • Change return type of getTokenAccountBalance and getTokenSupply to struct containing ui_amount (f64), amount (string representation of u64), and decimals (u8).
  • Change getTokenLargestAccounts amount field to same struct
  • Fixup native-mint handling across all token rpcs

Fixes #11366

@CriesofCarrots CriesofCarrots requested a review from mvines August 4, 2020 23:59
@mvines
Copy link
Member

mvines commented Aug 5, 2020

Looks good, let me try to integrate this into the token cli locally first. I'll respond back here hopefully later tonight

@codecov
Copy link

codecov bot commented Aug 5, 2020 •
edited
Loading

Codecov Report

Merging #11370 into master will increase coverage by 0.0%.
The diff coverage is 85.3%.

@@           Coverage Diff           @@
##           master   #11370   +/-   ##
=======================================
  Coverage    82.3%    82.3%           
=======================================
  Files         315      315           
  Lines       74224    74266   +42     
=======================================
+ Hits        61092    61129   +37     
- Misses      13132    13137    +5

mvines
mvines reviewed Aug 5, 2020
View reviewed changes
core/src/rpc.rs Outdated
})
.map(|account: &mut TokenAccount| account.amount)?;
})?;
let mint_account = bank
Copy link
Member

Choose a reason for hiding this comment

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

We need to check for native_mint::id() and then use native_mint::DECIMALS to avoid hitting "Invalid param: could not find mint"

mvines added a commit to mvines/solana-program-library that referenced this pull request Aug 5, 2020
@mvines
Adapt to solana-labs/solana#11370
3883174
@mvines
Copy link
Member

mvines commented Aug 5, 2020

Overall looks good, this PR allowed me to cut out a bunch of client code: solana-labs/solana-program-library@3883174

@CriesofCarrots
Copy link
Contributor Author

@mvines , do you want the rpc_client balance/supply methods to return the float only? or the complete amount struct?

mvines
mvines reviewed Aug 5, 2020
View reviewed changes
client/src/rpc_response.rs Outdated Show resolved Hide resolved
mvines
mvines reviewed Aug 5, 2020
View reviewed changes
client/src/rpc_response.rs Outdated Show resolved Hide resolved
mvines added a commit to mvines/solana-program-library that referenced this pull request Aug 5, 2020
@mvines
Adapt to solana-labs/solana#11370
b7e89d8
@mvines
Copy link
Member

mvines commented Aug 5, 2020

I've adapted solana-labs/solana-program-library#188 to the latest here. Working great, save for the native_mint issues we've discussed

@CriesofCarrots
Copy link
Contributor Author

@mvines , can you take the native-mint handling for a spin?

@mvines
Copy link
Member

mvines commented Aug 5, 2020

@mvines , can you take the native-mint handling for a spin?

I just did, it works well!

mvines
mvines previously approved these changes Aug 5, 2020
View reviewed changes
account-decoder/src/parse_token.rs Show resolved Hide resolved
client/src/rpc_response.rs Outdated Show resolved Hide resolved
core/src/rpc.rs Outdated Show resolved Hide resolved
docs/src/apps/jsonrpc-api.md Outdated Show resolved Hide resolved
@mergify mergify bot dismissed mvines’s stale review August 5, 2020 05:39

Pull request has been modified.

@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Aug 5, 2020
@mergify mergify bot merged commit 86e3f96 into solana-labs:master Aug 5, 2020
mergify bot pushed a commit that referenced this pull request Aug 5, 2020
@CriesofCarrots
Return token amounts as floats (#11370)
dc17782 
* Return token amounts as floats

* Floating-point equality

* Return float and raw token amounts

* Fix decimals and token rpcs for native-mint tokens

* Fixup docs and review comments

(cherry picked from commit 86e3f96)

# Conflicts:
#	core/src/rpc.rs
@mergify mergify bot mentioned this pull request Aug 5, 2020
Merged
mergify bot added a commit that referenced this pull request Aug 5, 2020
@mergify @CriesofCarrots
Return token amounts as floats (bp #11370) (#11378)
80525ac 
* Return token amounts as floats (#11370)

* Return token amounts as floats

* Floating-point equality

* Return float and raw token amounts

* Fix decimals and token rpcs for native-mint tokens

* Fixup docs and review comments

(cherry picked from commit 86e3f96)

# Conflicts:
#	core/src/rpc.rs

* Fix conflicts

Co-authored-by: Tyera Eulberg <[email protected]>
Co-authored-by: Tyera Eulberg <[email protected]>
@CriesofCarrots CriesofCarrots mentioned this pull request Aug 6, 2020
Merged
@CriesofCarrots CriesofCarrots deleted the token-decimals branch September 1, 2020 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvines mvines mvines left review comments

Assignees
No one assigned
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RPC: getTokenAccountBalance/getTokenSupply should normalize the amount based on the mint's decimals value
2 participants
@CriesofCarrots @mvines