0xPolygonMiden / miden-client

Client library that facilitates interaction with the Miden rollup
MIT License
25 stars 22 forks source link

feat: add custom clap parser for fungible assets #349

Closed mFragaBA closed 1 month ago

mFragaBA commented 1 month ago

closes #340

We replace the cli command options --amount and --faucet-id (or similar) with a single asset option. Now fungible assets are provided with the formats:

Discussion

UPDATE: for now we'll only parse into AccountId. Once we need it in the future we'll refactor as needed

mFragaBA commented 1 month ago

Just tried the command miden mint --target 0x8958d9acace0c89c --asset 10::BTC --note-type public (with a BTC faucet previously created) and got:

Input account ID {account_id} is neither a valid Account ID nor a prefix of a known Account ID

The command miden mint --target 0x8958d9acace0c89c --asset 10::0xa51b644392655b83 --note-type public (using the account id) works fine.

It won't work because we don't actually support token symbols yet (you'd get the same error if you had used --faucet BTC. You can notice the difference by doing 10:btc:

image

That is why I also mention on the readme that perhaps we should just allow account IDs for now (or at least we shouldn't mention it on the documentation yet)

tomyrd commented 1 month ago

It won't work because we don't actually support token symbols yet (you'd get the same error if you had used --faucet BTC. You can notice the difference by doing 10:btc:

Oh, my bad. Regardless I think there's a problem with the error message. Shouldn't the "{account_id}" be replaced by "BTC"?

mFragaBA commented 1 month ago
* While we are able to parse token symbols, I'm not sure if we should allow to do so until [Add token mapping to the client to show assets in a more user-friendly way #258](https://github.com/0xPolygonMiden/miden-client/issues/258) gets done.

I ended up parsing token symbols internally but removed any mention of using them in both the cli-reference doc and the CLI help. Once we add support for token symbols as mentioned in #258 we can mention them as well.

* upon seing these changes, I wonder if it's better to only allow either token symbol or full account IDs and not allow hex prefixes since the token symbol can already serve as a short way of refering to the faucet anyways

I think again for now we can keep it as is since we don't actually allow token symbols yet, but once we add token symbol support I think it's better to only accept either a token symbo or a full account id hex