-
Notifications
You must be signed in to change notification settings - Fork 168
Add token API endpoint for total supply
#3877
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
base: main
Are you sure you want to change the base?
Conversation
| @@ -0,0 +1,3 @@ | |||
| [route.total_minted_supply] | |||
| PATH = ["total_minted_supply"] | |||
| DOC = "Get the total supply of the espresso token, counting only tokens that have been minted. Notably, this does not include unminted tokens from (for example) unclaimed rewards, or other sources." | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment about the caching to make it clear that this doesn't always reflect the exact on-chain value immediately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And mention it's in token units with an example like "123.123"?
| impl<N: ConnectedNetwork<PubKey>, V: Versions, P: SequencerPersistence> TokenDataSource<SeqTypes> | ||
| for ApiState<N, P, V> | ||
| { | ||
| async fn get_total_supply(&self) -> anyhow::Result<U256> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the naming could be a bit more descriptive to make it clearer that this is the total supply on L1 and distinguish it from the total supply in Espresso. For example by adding an "l1" suffix.
sveitser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM but there are no tests.
There are a bunch of requirements we should test.
- Returns the correct values
- Caching / does not always call the provider.
If we don't add a test for caching at least a comment as to why we're caching would be nice.
| @@ -0,0 +1,3 @@ | |||
| [route.total_minted_supply] | |||
| PATH = ["total_minted_supply"] | |||
| DOC = "Get the total supply of the espresso token, counting only tokens that have been minted. Notably, this does not include unminted tokens from (for example) unclaimed rewards, or other sources." | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| DOC = "Get the total supply of the espresso token, counting only tokens that have been minted. Notably, this does not include unminted tokens from (for example) unclaimed rewards, or other sources." | |
| DOC = "Get the total supply of the espresso token, counting only tokens that have been minted on Ethereum. Notably, this does not include unminted tokens from (for example) unclaimed rewards, or other sources." |
No description provided.