Skip to content

Conversation

samkim-crypto
Copy link

@samkim-crypto samkim-crypto commented Jan 22, 2025

Problem

The spl-token-2022 crate version is still at 6.0.0 while 7.0.0 is out.

Summary of Changes

Updated the spl-token-2022 crate version to 7.0.0. The main logic changes are the following:

I will need to update the RPC as done in #1549, but this will require additional tests, so I decided to defer to an immediate follow-up PR and just focus on the token version bump here. Let me know if you prefer to add the RPC logic changes in this PR.

Fixes #

@samkim-crypto samkim-crypto marked this pull request as ready for review January 22, 2025 09:31
@samkim-crypto samkim-crypto requested a review from a team as a code owner January 22, 2025 09:31
@LucasSte
Copy link

LucasSte commented Jan 22, 2025

I'm not aware of the usages of floating points in scaled-ui. Comparing floating points directly (even if you convert them to strings) is problematic, since floating points carry rounding errors with them.

Depending on the context, you can define an epsilon for comparison, provided that you know the range these numbers will assume.

Have a look at: https://floating-point-gui.de/errors/comparison/

@samkim-crypto samkim-crypto marked this pull request as draft January 23, 2025 02:33
@samkim-crypto samkim-crypto marked this pull request as ready for review January 23, 2025 09:16
@samkim-crypto
Copy link
Author

@LucasSte I believe that the float values are not actually compared as numbers with UiScaledUiAmountConfig, so it seems that String is okay. @joncinque, do you have thoughts here?

Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks great! Just a question on using a string in instruction data

info: json!({
"mint": account_keys[account_indexes[0] as usize].to_string(),
"authority": authority.map(|pubkey| pubkey.to_string()),
"multiplier": f64::from(multiplier),

Choose a reason for hiding this comment

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

This is using a string elsewhere -- do you want to follow that same convention here?

Copy link
Author

Choose a reason for hiding this comment

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

Good point. Updated!

})?;
let mut value = json!({
"mint": account_keys[account_indexes[0] as usize].to_string(),
"newMultiplier": f64::from(multiplier),

Choose a reason for hiding this comment

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

Same here, string?

@joncinque
Copy link

@LucasSte I believe that the float values are not actually compared as numbers with UiScaledUiAmountConfig, so it seems that String is okay. @joncinque, do you have thoughts here?

That's correct, these numbers are not used for any logic, just for calculating a UI representation of the token amount. I think String makes the most sense

@LucasSte
Copy link

@LucasSte I believe that the float values are not actually compared as numbers with UiScaledUiAmountConfig, so it seems that String is okay. @joncinque, do you have thoughts here?

That's correct, these numbers are not used for any logic, just for calculating a UI representation of the token amount. I think String makes the most sense

Is leaving them out of the comparison an option? I'm afraid a comparison may error out for a silly rounding error.

@joncinque
Copy link

Is leaving them out of the comparison an option? I'm afraid a comparison may error out for a silly rounding error.

You mean the checks like this one in the test?

assert_eq!(token_amount.ui_amount_string, "1.051271096376024117");

Yeah sure, we could change the check to starts_with rather than full string equality. What do you think @samkim-crypto ?

@samkim-crypto
Copy link
Author

Is leaving them out of the comparison an option? I'm afraid a comparison may error out for a silly rounding error.

You mean the checks like this one in the test?

assert_eq!(token_amount.ui_amount_string, "1.051271096376024117");

Yeah sure, we could change the check to starts_with rather than full string equality. What do you think @samkim-crypto ?

Yeah I think I updated the tests to use starts_with, which seems like a good idea.

I think ideally, we would want the UiScaledUiAmountConfig type to not implement the Eq trait to prevent this type from being compared downstream (this will also allow us to just use f64 values for the multiplier). However, removing Eq from UiScaledUiAmountConfig will require that the UiExtension as well as all the other UI extension types to not implement the Eq type and this starts becoming a large change, so we probably don't want to go that route.

Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I'll let @LucasSte give the final approval

@samkim-crypto samkim-crypto merged commit 25a1e01 into anza-xyz:master Jan 27, 2025
58 checks passed
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.

3 participants