fix(core): render token uiAmount through the account decoder - #777
Conversation
Greptile SummaryThe PR replaces precision-losing and overflow-prone token amount formatting with the Solana account decoder’s exact string formatter and checked optional floating-point conversion.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (3): Last reviewed commit: "fix(core): render token uiAmount through..." | Re-trigger Greptile |
|
Thanks for the PR, @pucedoteth. Foundation PRs can't be merged without commit signature verification. If you can get that set up and re-push these commits, I'll review! |
83308bb to
174ca3c
Compare
MicaiahReid
left a comment
There was a problem hiding this comment.
I appreciate the PR, but in future PRs please do more to distill the AI output.
| /// The decimal rendering of a raw token amount, as agent-visible RPC output. | ||
| /// | ||
| /// Defers to `real_number_string_trimmed`, the same helper `getTokenSupply` uses since #758, | ||
| /// rather than dividing through `f64`. An `f64` carries 53 bits of mantissa and a token amount | ||
| /// carries 64, so every amount above 2^53 rounded: `u64::MAX` at 6 decimals rendered as | ||
| /// `18446744073709.550781` instead of `18446744073709.551615`. It also drops the | ||
| /// `10u64.pow(decimals)` that overflowed for `decimals >= 20` — `Mint::decimals` is a `u8` with | ||
| /// no cap in the token program, so that was a panic in debug and a wrong divisor in release. | ||
| pub fn format_ui_amount_string(amount: u64, decimals: u8) -> String { | ||
| if decimals > 0 { | ||
| let divisor = 10u64.pow(decimals as u32); | ||
| format!( | ||
| "{:.decimals$}", | ||
| amount as f64 / divisor as f64, | ||
| decimals = decimals as usize | ||
| ) | ||
| } else { | ||
| amount.to_string() | ||
| } | ||
| real_number_string_trimmed(amount, decimals) | ||
| } | ||
|
|
||
| pub fn format_ui_amount(amount: u64, decimals: u8) -> f64 { | ||
| if decimals > 0 { | ||
| let divisor = 10u64.pow(decimals as u32); | ||
| amount as f64 / divisor as f64 | ||
| } else { | ||
| amount as f64 | ||
| } | ||
| /// The `uiAmount` float, or `None` when `decimals` is too large to form a divisor. | ||
| /// | ||
| /// Matches `solana_account_decoder::parse_token::token_amount_to_ui_amount_v3`, which computes | ||
| /// this as `10_usize.checked_pow(decimals)` mapped over the division: the field is | ||
| /// `Option<f64>` precisely so an undivisible `decimals` has somewhere to go other than a panic. | ||
| pub fn format_ui_amount(amount: u64, decimals: u8) -> Option<f64> { | ||
| 10_usize | ||
| .checked_pow(decimals as u32) | ||
| .map(|divisor| amount as f64 / divisor as f64) |
There was a problem hiding this comment.
The doc comments on these are classic over-described AI generated comments. Please distill this down to what's important. What does the function do? If you had no context, what comment would help you know what the function does, when to use it, and what to pass in?
The comment doesn't need super technical details of what's happening under the hood (the code itself explains what the code is doing). We don't need references to other PRs and how past behavior worked
| /// `18446744073709.550781` instead of `18446744073709.551615`. It also drops the | ||
| /// `10u64.pow(decimals)` that overflowed for `decimals >= 20` — `Mint::decimals` is a `u8` with | ||
| /// no cap in the token program, so that was a panic in debug and a wrong divisor in release. | ||
| pub fn format_ui_amount_string(amount: u64, decimals: u8) -> String { |
There was a problem hiding this comment.
If this function is just directly calling another - why does the function need to exist at all?
| #[test] | ||
| fn test_format_ui_amount_string_matches_account_decoder() { | ||
| // `getTokenSupply` renders through `real_number_string_trimmed` since #758. These | ||
| // helpers feed `getTokenLargestAccounts` and the pre/post token balances on | ||
| // `getTransaction`, and must agree with it for the same mint. | ||
| for (amount, decimals) in [(0u64, 0u8), (1, 0), (1_000_000, 6), (1, 9), (u64::MAX, 18)] { | ||
| assert_eq!( | ||
| format_ui_amount_string(amount, decimals), | ||
| real_number_string_trimmed(amount, decimals), | ||
| "amount {amount} at {decimals} decimals must match the account decoder" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_format_ui_amount_survives_decimals_that_cannot_form_a_divisor() { | ||
| // `Mint::decimals` is an unvalidated `u8`, so a mint may carry any value up to 255. | ||
| // `10u64.pow(decimals)` overflowed from 20 up: a panic in debug, a wrong divisor in | ||
| // release. Both helpers must instead answer for the whole `u8` range. | ||
| for decimals in 0u8..=255 { | ||
| let rendered = format_ui_amount_string(1, decimals); | ||
| assert_eq!( | ||
| rendered, | ||
| real_number_string_trimmed(1, decimals), | ||
| "decimals {decimals} must render rather than overflow" | ||
| ); | ||
| assert_eq!( | ||
| format_ui_amount(1, decimals).is_some(), | ||
| 10_usize.checked_pow(decimals as u32).is_some(), | ||
| "decimals {decimals} must yield a float exactly when a divisor exists" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
These tests don't seem particularly valuable, they're essentially asserting the one function has the same output as another... when the first function is just calling the other
format_ui_amount_string divided through f64 before formatting, so amounts above 2^53 were rounded: u64::MAX at 6 decimals rendered 18446744073709.550781 rather than 18446744073709.551615. Both helpers also built their divisor with 10u64.pow(decimals), which overflows from decimals >= 20. Drop the string helper and call real_number_string_trimmed at the call sites, matching getTokenSupply. format_ui_amount returns Option<f64>, which is what UiTokenAmount::ui_amount already holds. Note: trailing zeros are no longer padded, so 1 USDC is "1" rather than "1.000000" — what agave returns and what getTokenSupply already did here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
174ca3c to
c203213
Compare
|
Fair thanks, that was too much. Addressed all three:
Diff is now +28/-31 instead of +125. One thing I left alone: |
MicaiahReid
left a comment
There was a problem hiding this comment.
thanks, @pucedoteth!
The bug
format_ui_amount_stringincrates/core/src/surfnet/locker.rsdivided throughf64before formatting:An
f64mantissa holds 53 bits; a token amount holds 64. Every amount above 2^53 was rounded before it reached the client:1844674407370955161518446744073709.55078118446744073709.5516151234567890123456712345678901.23456812345678901.234567900719925474099390071992547409.9290071992547409.93This is not a theoretical range. 2^53 raw units is only ~9B tokens on a 6-decimal mint, so an agent reading a supply-scale balance gets a wrong number back — the same class of defect #758 fixed for
getTokenSupply, in the helpers that path didn't touch.Both helpers additionally built their divisor with
10u64.pow(decimals), which overflows fromdecimals >= 20.Mint::decimalsis a plainu8with no cap in the token program, so a mint declaring 20+ decimals made this a panic in debug and a silently wrong divisor in release:Reachability
All four call sites pass real mint decimals, so both defects are live on RPC output:
locker.rs:3040—getTokenLargestAccountstypes.rs:516,:538,:732— pre/post token balances ongetTransactionandgetBlockThe fix
Defer the string to
real_number_string_trimmed— the helpergetTokenSupplyalready uses since a4acb1e (#758) — and giveformat_ui_amounttheOption<f64>return thatUiTokenAmount::ui_amountis already typed for. That mirrorssolana_account_decoder::parse_token::token_amount_to_ui_amount_v3exactly:The three
types.rscall sites drop their now-redundantSome(...)wrapper.One deliberate behaviour change: trailing zeros are no longer padded, so 1 USDC renders as
"1"rather than"1.000000". That is whatreal_number_string_trimmeddoes, so it is what agave returns on mainnet and whatgetTokenSupplyalready returned here after #758 — this brings the remaining paths into line rather than away from it.Verification
Four tests added to the existing
locker.rstest module. Reverting only the production helper body — tests untouched — turns three of them red with exactly the values above:All four pass with the fix. Full
cargo test -p surfpool-core --lib: 716 passed. Thetest_simulate_add_alt_entries_fetchingvariants fail identically on a cleanmainat this commit (loaded_accounts_data_size6090 vs 140134 — it fetches from mainnet), so they are pre-existing and unrelated.cargo fmtandcargo clippyclean on the changed lines.🤖 Generated with Claude Code