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.

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

feat: make MAX_DECIMALS public #1063

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

SantiagoPittella
Copy link
Collaborator

We need to use this values in the miden-client repo to avoid repetition. 0xPolygonMiden/miden-client#476

Copy link
Collaborator

@igamigo igamigo 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! Left a minor nit. I'm not sure we need the max supply constant to be public but it probably does not hurt to expose anyway.

/// The maximum value for the `max_supply` field.
pub const MAX_MAX_SUPPLY: u64 = (1 << 63) - 1;

/// The maximum number of decimals supported by the component.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this enforced at the protocol-level? Because if so we could write something like this (same for above):

Suggested change
/// The maximum number of decimals supported by the component.
/// The maximum number of decimals supported by faucets in the protocol.

cc @PhilippGackstatter @Fumuran

Copy link
Contributor

Choose a reason for hiding this comment

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

MAX_DECIMALS is not enforced at the protocol level.

@SantiagoPittella
Copy link
Collaborator Author

I'm not really happy with the result here. Maybe having these constants in the lib.rs is better.

@SantiagoPittella SantiagoPittella changed the title feat: make MAX_DECIMALS and MAX_MAX_SUPPLY public feat: make MAX_DECIMALS public Jan 15, 2025
Copy link
Contributor

@bobbinth bobbinth 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! Thank you!

@SantiagoPittella SantiagoPittella force-pushed the santiagopittella-make-max-decimals-public branch from 8488e8a to 1c4b4c5 Compare January 15, 2025 18:29
@SantiagoPittella SantiagoPittella merged commit ff3d01e into next Jan 15, 2025
11 checks passed
@SantiagoPittella SantiagoPittella deleted the santiagopittella-make-max-decimals-public branch January 15, 2025 18:50
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