-
Notifications
You must be signed in to change notification settings - Fork 0
feat: implement mint command and rename binaries #196
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
Conversation
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.
Overall looks good, I tested on my machine a couple of times* and managed to mint tokens, here's the midenscan link.
I think my main question is if whether encapsulate the functionality in a separate library and also a separate binary. Would love to hear other thoughts!
- save for this small issue
bin/faucet/src/main.rs
Outdated
| mod api_key; | ||
| mod frontend; | ||
| mod logging; | ||
| mod mint; |
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.
What if the mint functionality was implemented on a separate binary/crate, rather than adding it in the same binary as the other faucet utilities. Mainly since, minting/interacting and serving a faucet are fairly distinct from each other.
This was briefly mentioned in the original discussion, but what if the main bulk of the logic was implemented inside the faucet's library (or maybe even a separate, smaller library altogether) and then the actual command simply served as a wrapper to said library. I believe this library could potentially serve other uses in the future, besides minting.
What do you think?
I'm also tagging @TomasArrachea here.
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.
What if the mint functionality was implemented on a separate binary/crate, rather than adding it in the same binary as the other faucet utilities. Mainly since, minting/interacting and serving a faucet are fairly distinct from each other.
Completely agree, I was going to suggest the same thing
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 imagine the split along the lines of: miden-faucet-{operator/admin/setup} and miden-faucet-client (names TBD)
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.
Yeah I think that could make sense. How would you go about naming that library, especially since you said it could serve other use cases too?
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.
Yeah I think that could make sense. How would you go about naming that library, especially since you said it could serve other use cases too?
Great question, my first instinct was going with miden-faucet-client since it would contain the "client side" logic; but maybe people who are more involved with the faucet have better insight.
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.
Will code out the split!
bin/faucet-client/src/mint.rs
Outdated
| .await?; | ||
|
|
||
| println!("Mint request accepted. Transaction: {}", minted_note.tx_id); | ||
| println!("Public P2ID note commitment: {}", minted_note.note_id.to_hex()); |
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.
Just a heads up (related to @igamigo comment); midenup will probably need a way to communicate values in between binaries that are run in succession.
In this case, the minted note's ID will need to be passed down to the subsequent miden-client consume-notes call.
My first guess would be to pipe specific values in between binaries, but it's not yet defined.
Like I said, just a heads up.
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.
Thanks for bringing this to my attention. I think we should definitely think about that within the scope of this PR too, as it may have implications here.
I will work on adding functionality for the multi-command single aliases on midenup, I think while doing that we may want to standardize how midenup communicates between other binaries.
| /// Amount to mint (in base units). | ||
| #[arg(short = 'm', long = "amount", value_name = "U64")] | ||
| amount: u64, |
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.
Extremely minor nit, but what if instead of amount we used the word quantity as a flag? In order to have distinct short form versions of the command.
This would enable the use of -a for Account and -q for Quantity instead of -m for aMount.
"Amount" could be left as an alias as to follow tradition, like so:
| /// Amount to mint (in base units). | |
| #[arg(short = 'm', long = "amount", value_name = "U64")] | |
| amount: u64, | |
| /// Quantity to mint (in base units). | |
| #[arg(short = 'q', long = "quantity", value_name = "U64", alias = "amount")] | |
| quantity: u64, |
With this, both amount and quantity would both work (although the "amount" alias would be hidden).
Like I said, it's just a small suggestion/nit. Especially if -m is used elsewhere, then by all means ignore this 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.
Yeah I think that's a good idea to avoid using -m, will add this!
bin/faucet-client/src/mint.rs
Outdated
| /// Response from the `/pow` endpoint. | ||
| #[derive(Debug, Deserialize, Serialize, Clone)] | ||
| struct PowResponse { | ||
| challenge: String, | ||
| target: u64, | ||
| } | ||
|
|
||
| /// Response from the `/get_tokens` endpoint. | ||
| #[derive(Debug, Deserialize, Serialize, Clone)] | ||
| struct GetTokensResponse { | ||
| note_id: String, | ||
| tx_id: String, | ||
| } |
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.
If we follow the path of creating a new separate library, then maybe these structs could go there as well.
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.
Agreed!
| fn parse_account_id(input: &str) -> Result<AccountId, MintClientError> { | ||
| if input.starts_with("0x") { | ||
| AccountId::from_hex(input) | ||
| .map_err(|err| MintClientError::InvalidAccount(input.to_owned(), err.to_string())) | ||
| } else { | ||
| Address::decode(input) | ||
| .map_err(|err| MintClientError::InvalidAccount(input.to_owned(), err.to_string())) | ||
| .and_then(|(_, address)| match address.id() { | ||
| AddressId::AccountId(account_id) => Ok(account_id), | ||
| _ => Err(MintClientError::InvalidAccount( | ||
| input.to_owned(), | ||
| "address is not account-based".to_owned(), | ||
| )), | ||
| }) | ||
| } | ||
| } |
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 just tried executing the mint command without passing 0x and got the following error:
$~ miden-faucet mint --amount 88 --account 84384f2e124c0490458bf6d899503f
Error: invalid account `84384f2e124c0490458bf6d899503f`: failed to decode bech32 string into an addressHaving said that, the following patch does work on my machine:
| fn parse_account_id(input: &str) -> Result<AccountId, MintClientError> { | |
| if input.starts_with("0x") { | |
| AccountId::from_hex(input) | |
| .map_err(|err| MintClientError::InvalidAccount(input.to_owned(), err.to_string())) | |
| } else { | |
| Address::decode(input) | |
| .map_err(|err| MintClientError::InvalidAccount(input.to_owned(), err.to_string())) | |
| .and_then(|(_, address)| match address.id() { | |
| AddressId::AccountId(account_id) => Ok(account_id), | |
| _ => Err(MintClientError::InvalidAccount( | |
| input.to_owned(), | |
| "address is not account-based".to_owned(), | |
| )), | |
| }) | |
| } | |
| } | |
| let account_id = if !input.starts_with("0x") { | |
| format!("0x{input}") | |
| } else { | |
| String::from(input) | |
| }; | |
| AccountId::from_hex(&account_id) | |
| .map_err(|err| MintClientError::InvalidAccount(input.to_owned(), err.to_string())) | |
Does that suffice? This also removes the need to import Address and AddressId I believe.
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.
The exact code you provided will not work anymore for bech32 account IDs. We can combine your proposed check with the current function. The "0x" check exists to differentiate the account ID from a bech32 string and hex format.
I borrowed this logic from the miden-client utils. As I mentioned in the PR description, I think we may want to consider moving it to miden-base, since both this repository and the miden-client use it.
Nevertheless, if we do want to change the function to support hex strings without a "0x" prefix, I think we should also change the logic in the miden-client helper, since it also applies there.
@mmagician Would love to hear your thoughts on this!
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.
implementing TryFrom<String> for AccountId would be a good addition to miden-base
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 agree
|
Hey everyone, I made a commit to address the binary split that was discussed above. Now the repository contains two disticnt binaries:
Remaining open questions:
|
171e1ee to
4c28193
Compare
Cargo.toml
Outdated
| @@ -1,5 +1,5 @@ | |||
| [workspace] | |||
| members = ["bin/faucet", "crates/faucet"] | |||
| members = ["bin/faucet", "bin/faucet-operator", "crates/faucet"] | |||
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'd rename the directories too, to align with operator - client split
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.
Absolutely agree, I implemented that just now!
README.md
Outdated
|
|
||
| ```bash | ||
| miden-faucet init \ | ||
| miden-faucet-client init \ |
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.
this is for the operator, no?
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.
hmm I see, my idea was the opposite, i.e. keeping the current binary as the operator/admin binary, and adding a new client binary.
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.
Sorry for misreading your guys's comment. I changed the naming to be what you guys proposed.
README.md
Outdated
| Requesting tokens from a faucet endpoint is handled by the separate `miden-faucet-operator` binary: | ||
| ```bash |
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.
So this (in and other places) would be reversed
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.
Absolutely agree, I refactored the markdown documentation to adjust for that.
bin/faucet-client/src/mint.rs
Outdated
| @@ -0,0 +1,309 @@ | |||
| //! CLI command to mint tokens from a remote faucet by solving its `PoW` challenge. | |||
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.
technically this is not minting, because the faucet only has mint capability exposed to the faucet owner. Rather, I'd say "requesting"
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.
this makes me wonder whether we should update not just the docs, but the name of the command itself
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.
Although I do agree that "requesting" is more technically accurate, I think it would be less intuitive for users, also I think coming up with a good name for that CLI command is harder, i.e. request-asset?
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.
Although I do agree that "requesting" is more technically accurate, I think it would be less intuitive for users, also I think coming up with a good name for that CLI command is harder, i.e. request-asset?
I agree with mint being both more intuitive and nicer in the CLI.
In this case, let's make sure that the accurate description is reflected in the docs, i.e. we write that it's not technically a mint.
| fn parse_account_id(input: &str) -> Result<AccountId, MintClientError> { | ||
| if input.starts_with("0x") { | ||
| AccountId::from_hex(input) | ||
| .map_err(|err| MintClientError::InvalidAccount(input.to_owned(), err.to_string())) | ||
| } else { | ||
| Address::decode(input) | ||
| .map_err(|err| MintClientError::InvalidAccount(input.to_owned(), err.to_string())) | ||
| .and_then(|(_, address)| match address.id() { | ||
| AddressId::AccountId(account_id) => Ok(account_id), | ||
| _ => Err(MintClientError::InvalidAccount( | ||
| input.to_owned(), | ||
| "address is not account-based".to_owned(), | ||
| )), | ||
| }) | ||
| } | ||
| } |
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.
implementing TryFrom<String> for AccountId would be a good addition to miden-base
a09ce2c to
5df3e72
Compare
d54a4a0 to
6c20237
Compare
|
Hey guys, so I did another round of refactoring:
|
bobbinth
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.
Looks good! Thank you! Not a full review, but I left some comments inline.
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.
Should we make some minor changes in this file to add the "operator" terminology?
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.
Agreed, I will make it clear in the README that the faucet comes with two distinct binaries, and explain the purpose of each of those.
README.md
Outdated
| Requesting tokens from a faucet endpoint is handled by the separate `miden-faucet-client` binary: | ||
| ```bash | ||
| miden-faucet-client mint --url <FAUCET_API_URL> --account <ACCOUNT_ID> --amount <BASE_UNITS> | ||
| ``` |
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.
It feels like maybe this deserves its own small sub-section.
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.
Agreed! I will implement a sub-section
| ## Step 3: Request Test Tokens | ||
|
|
||
| Once the faucet is running, you can request test tokens through either the web interface or the REST API. | ||
| Once the faucet is running, you can request test tokens through either the web interface, the operator CLI, or the REST API. |
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.
nit: should "operator CLI" actually be "client CLI"?
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 these are still leftovers from #196 (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.
Yes, sorry this is still a leftover from my last refactoring. Will go through the docs files again to make sure that everything is named correctly!
docs/src/getting-started/cli.md
Outdated
| To request tokens from a remote faucet, use the client mint CLI: | ||
| ```bash | ||
| miden-faucet-client mint --url <FAUCET_API_URL> --account <ACCOUNT_ID> --amount <BASE_UNITS> | ||
| ``` |
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.
It feels like this should be in a separate small section. Also, we should mention somewhere closer to the top that there are actually two separate binaries.
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.
Absolutely agreed, will add that and amend that!
bin/faucet-client/src/mint.rs
Outdated
| @@ -0,0 +1,309 @@ | |||
| //! CLI command to mint tokens from a remote faucet by solving its `PoW` challenge. | |||
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.
Although I do agree that "requesting" is more technically accurate, I think it would be less intuitive for users, also I think coming up with a good name for that CLI command is harder, i.e. request-asset?
I agree with mint being both more intuitive and nicer in the CLI.
In this case, let's make sure that the accurate description is reflected in the docs, i.e. we write that it's not technically a mint.
mmagician
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.
LGTM thanks!
feat: rename existing miden-faucet binary to miden-faucet-client docs: update docs to include information about new miden-faucet-operator binary chore: move tests of miden-faucet-operator binary into dedicated folder docs(README): update README for binary separation chore: update changelog
docs: update instructions to include correct client start command and mint command usage
…client (reverse naming)
fix(Makefile): install faucet instructions target correct binary docs(README): added explainer for binary split chore: removed redundant readme key in bin/faucet-client/Cargo.toml chore: amend description satement in bin/faucet-operator/Cargo.toml
docs: Clarify mint command technical accuracy
…ries feat: implement backwards compatibility for legacy "miden-faucet" CLI command
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.
LGTM! I think @sergerad should probably look into this as it changes some things in relation to how the faucet service was being set up before
sergerad
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.
Approved but I think renaming faucet binary to faucet-operator has added work for no value. There will need to be a few small changes in the infrastructure repo following this PR also.
I'm happy to revert that change so that we keep the original naming the
@mmagician What are your thoughts on that? @sergerad mentioned to me via Slack that you @igamigo would also be in favour of reverting the naming to the original, correct? |
That works, sure 👍🏼 I think the important part is having the user-facing code under a |
I agree! |
|
Alright, I went ahead and reverted the faucet binary name from |
mmagician
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.
LGTM with a few leftover naming changes
Co-authored-by: Marti <[email protected]>
Co-authored-by: Marti <[email protected]>
Co-authored-by: Marti <[email protected]>
|
@Keinberger, this PR was merged to |
Correct, plan would be a patch release. I will do a PR on Sorry about forgetting to squash merge this... Won't happen again! As for the target of 0xMiden/miden-client#1642 - why would we need to change it? If we do a patch release I suppose it also belongs on |
|
FYI I'm also fine with merging everthing onto |
No need to merge to |
|
It would be the next relase, v0.13, right? The plan is to release v0.13 of the client tomorrow or Wednesday. This PR was already merged to |
|
Yes, I blindly said 0.14 but meant 0.13, sorry. This PR was merged to |
Brings mint command feature (#196) from main to next. Key changes: - Added new miden-faucet-client binary with mint command - Updated all documentation to use next's parameter conventions (api-bind-port, frontend-bind-port, no-frontend) - Moved mint feature to v0.13.0 in CHANGELOG (not patch release) - Regenerated Cargo.lock for next's dependencies Resolved conflicts: - Cargo.lock: Regenerated from scratch - docs/src/getting-started/quick-start.md: Used next's simplified parameters Note: Build requires miden-client compatibility fixes (separate work)
Brings mint command feature (#196) from main to next. Key changes: - Added new miden-faucet-client binary with mint command - Updated all documentation to use next's parameter conventions (api-bind-port, frontend-bind-port, no-frontend) - Moved mint feature to v0.13.0 in CHANGELOG (not patch release) - Regenerated Cargo.lock for next's dependencies Resolved conflicts: - Cargo.lock: Regenerated from scratch - docs/src/getting-started/quick-start.md: Used next's simplified parameters Note: Build requires miden-client compatibility fixes (separate work)
Brings mint command feature (#196) from main to next. Key changes: - Added new miden-faucet-client binary with mint command - Updated all documentation to use next's parameter conventions (api-bind-port, frontend-bind-port, no-frontend) - Moved mint feature to v0.13.0 in CHANGELOG (not patch release) - Regenerated Cargo.lock for next's dependencies Resolved conflicts: - Cargo.lock: Regenerated from scratch - docs/src/getting-started/quick-start.md: Used next's simplified parameters Note: Build requires miden-client compatibility fixes (separate work)
Summary
This PR implements a
mintCLI command, that users can use to request a public P2ID note from a faucet, i.e. "mint" tokens from a faucet.Furthermore, this PR introduces a new
miden-faucet-clientbinary, which implements the "mint" functionality. The reasoning as to why this decision was made is described in more detail at the "Binary refactoring" section below. This explains the large amount of file changes, although most are just due to renaming the existing "faucet" binary folder.The
mintcommand is part of themiden mintcommand that will be implemented atmidenup. See this comment here on how themiden mintcommand will work onmidenup: #192 (comment)This feature was discussed on this issue: #192
Usage
The new command can be used as follows:
Note that the
--urlparameter is optional, the faucet will default to the testnet faucet URL if the flag has not been provided.Open Questions
There is a helper function that I implemented here: I borrowed this helper function from the code present at the
miden-clientrepository. Since bothmiden-clientandmiden-faucetwill now use the same logic, I think it might make sense to include that function inmiden-baseso that both repositories can import it from there. Let me know about your thoughts!I placed the tests inside of the
miden-faucet-clientinto a dedicatedtests/folder, which speaks against the fashion of how tests are laid out across the rest of the faucet repository. I think it's still better in terms of code organization to have them in a dedicatedtests/´ folder, but happy to move them inside of themint.rs` file directly if that's what we prefer.Closes #192
EDITS
CI
action.ymlfile changesAs part of this PR, I made changes to the
action.ymlfile workflow to make sure that it references all crate, file and directory names correctly (since some have been renamed). Checking these changes for accuracy is still pending. I have outlined those changes in the following comment: #196 (comment)Request Struct exports
As outlined in the following comment, the PR also changes the
faucet-operatorto export request structs that are used by thefaucet-clientfor making requests to a faucet: #196 (comment)Binary refactoring
The new
mintcommand is implemented inside of a dedicated binary calledmiden-faucet-client.The two libraries will and are responsible for holding distinct logic:
miden-faucet: Existingmiden-faucetbinary.miden-faucet-client: This one contains the mint logic (and any future logic we may want to add to interact with live faucets). Not sure if the naming is perfect, let me know if you have any other proposals!This has been outlined in the following comment: #196 (comment)
Throughout the development of this PR, the faucet binary was temporarily renamed to
miden-faucet-operator. However, after discussion with reviewers, we've reverted this change to maintain backward compatibility. The final implementation keeps the originalmiden-faucetbinary name unchanged, while adding the newmiden-faucet-clientbinary. This avoids unnecessary infrastructure changes and deployment work. See comment: #196 (comment)