Skip to content

native-token: Deprecate float usage, add str usage#171

Merged
joncinque merged 1 commit intoanza-xyz:masterfrom
joncinque:deprecatefloat
May 14, 2025
Merged

native-token: Deprecate float usage, add str usage#171
joncinque merged 1 commit intoanza-xyz:masterfrom
joncinque:deprecatefloat

Conversation

@joncinque
Copy link
Copy Markdown
Collaborator

Problem

The sol_to_lamports and lamports_to_sol functions are problematic because they use floats, which are not precise.

Summary of changes

Deprecate the functions, and add versions that use strings instead.

Added @tao-stones to check the FeeStructure change -- it looks like FeeStructure::new is only used in one test.

#### Problem

The `sol_to_lamports` and `lamports_to_sol` functions are problematic
because they use floats, which are not precise.

#### Summary of changes

Deprecate the functions, and add versions that use strings instead.
@joncinque joncinque requested review from rustopian and tao-stones May 14, 2025 10:47
@joncinque joncinque enabled auto-merge (squash) May 14, 2025 10:47
Copy link
Copy Markdown
Contributor

@rustopian rustopian left a comment

Choose a reason for hiding this comment

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

Correct, good tests, all works great.

@joncinque joncinque merged commit 84954b4 into anza-xyz:master May 14, 2025
24 checks passed
@rustopian
Copy link
Copy Markdown
Contributor

rustopian commented May 14, 2025

Oops, @tao-stones your review is still requested directly, my bad on triggering auto-merge

@t-nelson
Copy link
Copy Markdown
Contributor

i do this far too often 🤦

Screenshot from 2025-05-14 19-30-55

@joncinque joncinque deleted the deprecatefloat branch May 15, 2025 09:51
@joncinque
Copy link
Copy Markdown
Collaborator Author

joncinque commented May 15, 2025

Ah darn, sorry -- for the first point, we'll get a warning on removal, which will fail CI, so we can't forget to remove the const.

And for the second point, I wasn't aware of that function... I guess the options are to either include a bool flag (edit: in this new function), or just remove it and tell people to use build_balance_message. All of the users of lamports_to_sol are CLIs / user-facing, so dependencies aren't an issue. I'd lean towards the second option. What do you think?

Copy link
Copy Markdown
Contributor

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

Changes to fee-structure is good.

@t-nelson
Copy link
Copy Markdown
Contributor

t-nelson commented May 15, 2025

Ah darn, sorry

no worries! i'm the idiot that didn't hit the comment button

for the first point, we'll get a warning on removal, which will fail CI, so we can't forget to remove the const.

👍

And for the second point, I wasn't aware of that function... I guess the options are to either include a bool flag (edit: in this new function), or just remove it and tell people to use build_balance_message. All of the users of lamports_to_sol are CLIs / user-facing, so dependencies aren't an issue. I'd lean towards the second option. What do you think?

yeah agreed. i think we should stick one implementation to avoid all of the caveats of replicode.

... though i could be convinced that build balance message belongs in a different crate 🤔

@joncinque
Copy link
Copy Markdown
Collaborator Author

Well, I put in #174 so we can discuss there

febo pushed a commit to febo/solana-sdk that referenced this pull request Sep 21, 2025
#### Problem

The `sol_to_lamports` and `lamports_to_sol` functions are problematic
because they use floats, which are not precise.

#### Summary of changes

Deprecate the functions, and add versions that use strings instead.
grod220 pushed a commit that referenced this pull request Mar 9, 2026
* remove `OptionalNonZeroElGamalPubkey` in `spl-pod`

* add `derive` feature on serde

* cargo fmt
grod220 pushed a commit that referenced this pull request Mar 16, 2026
* remove `OptionalNonZeroElGamalPubkey` in `spl-pod`

* add `derive` feature on serde

* cargo fmt
grod220 pushed a commit that referenced this pull request Mar 18, 2026
* remove `OptionalNonZeroElGamalPubkey` in `spl-pod`

* add `derive` feature on serde

* cargo fmt
grod220 pushed a commit that referenced this pull request Mar 19, 2026
* remove `OptionalNonZeroElGamalPubkey` in `spl-pod`

* add `derive` feature on serde

* cargo fmt
grod220 pushed a commit that referenced this pull request Mar 19, 2026
* remove `OptionalNonZeroElGamalPubkey` in `spl-pod`

* add `derive` feature on serde

* cargo fmt
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.

4 participants