Skip to content

Conversation

@tdelabro
Copy link
Contributor

@tdelabro tdelabro commented Oct 13, 2025

Rust formatting for hex representation used the alternate formatted attribute to decide if the 0x prefix should be used or not.
You are not doing it.
This causes an issue downstream in https://github.com/starknet-io/types-rs, forcing me to do intermediate string allocation to strip the prefix and then really it if actually specified in the formater

See: https://github.com/starknet-io/types-rs/blob/main/crates/starknet-types-core/src/felt/alloc_impls.rs#L29-L35

Also, this is a breaking change, as all the previous uses without {:#} will now print without 0x, and only the ones with {:#} will print 0x...
So maybe we want to do the opposite: 0x being the default and no 0x being the alternate. It's a bit quirky, as it opposes the way Rust lang's LowerHex and UpperHex traits handle this, but it would still work.

Also, I fixed this single instance because it is the one I depend on, but it may be a more widespread issue across the rest of your codebase.

Also, you could probably implement LowerHex and UpperHex and handle sign, padding, alternate, etc correctly

@tdelabro tdelabro requested a review from a team as a code owner October 13, 2025 18:08
Copy link
Collaborator

@diegokingston diegokingston left a comment

Choose a reason for hiding this comment

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

There is an inconsistency for no-std. Fixing that should allow us to merge it

@diegokingston diegokingston self-requested a review October 14, 2025 20:00
@tdelabro tdelabro force-pushed the fix-altenate-display branch 2 times, most recently from e61ab98 to 410c8f3 Compare October 14, 2025 22:17
@tdelabro
Copy link
Contributor Author

tdelabro commented Oct 14, 2025

Hey, I added LowerHex and UpperHex requirements to the trait IsUnsignedInteger.
It really helps later on in from_reduced_big_uint, by not having to guess if the generic type passed used hex or decimal, it's Display implementation. We always use LowerHex without prefix instead.
Less code, better performance.

Display actually doesn't have an alternate form according to the Rust docs, so we always go for 0x... This is arbitrary, but coherent with the previous implementation (which makes it a non-breaking change after all).
For Lower/UpperHex, we handle the alternate flag accordingly.
This way, everything is covered and up to rust standards

edit: it is actually a breaking change because the public IsUnsignedInteger type now has increased requirement.

@tdelabro tdelabro requested a review from pablodeymo October 14, 2025 22:19
@tdelabro tdelabro force-pushed the fix-altenate-display branch from 410c8f3 to 92cd943 Compare October 15, 2025 04:31
@tdelabro
Copy link
Contributor Author

@pablodeymo @diegokingston wdyt of the new impl?

Copy link
Contributor

@pablodeymo pablodeymo left a comment

Choose a reason for hiding this comment

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

🚀

@diegokingston
Copy link
Collaborator

Looks good! We need all the commits to be signed and we can merge it!

@tdelabro
Copy link
Contributor Author

@diegokingston my commits are not signed by default? How do I fix this?

Also, codecov failed, probably because I'm pushing from a fork, wdyt?

@diegokingston
Copy link
Collaborator

I guess you need to configure your signing key https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits or check if you're using one password https://developer.1password.com/docs/ssh/git-commit-signing/. You have a way of signing everything "git rebase --exec 'git commit --amend --no-edit -n -S' -i main" though you may need to use "git push --force-with-lease". Uploading codecov is not a problem, that's something I can solve on our side

@tdelabro tdelabro force-pushed the fix-altenate-display branch from 92cd943 to 3eca081 Compare October 17, 2025 18:09
@tdelabro
Copy link
Contributor Author

@diegokingston done, my commits are signed and verified

@diegokingston diegokingston merged commit 3b96e92 into lambdaclass:main Oct 17, 2025
7 of 8 checks passed
@tdelabro tdelabro deleted the fix-altenate-display branch October 17, 2025 19:24
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