Fix None derivation path index for custom currencies
#464
Conversation
You can't currently run a mint using a custom currency unit because it throws Error::UnsupportedUnit when getting the derivation path in Mint::new. This fix allows the creator of a custom CurrencyUnit to specify the derivation path. It formats these custom currency units into strings like UNIT:PATH where UNIT is the currency name string and PATH is the derivation path int.
derivation paths 0-3 are already assigned unit test this behavior also add a unit test for derivation_path_from_unit which was where the custom currency bug manifested
vnprc
changed the title
None derivation path for custom currenciesNone derivation path index for custom currencies
There was a problem hiding this comment.
Thanks for the PR is this an area of the code base I think needs some improvement. However, we can't create our own encoding for custom units with their paths as other implementations will not be able to handle them and we will not be able to handle units from other implementations that don't include the derivation path in the unit.
You can start a Mint with a custom unit you just also have to include the derivation path. Though I don't love this solution as failing to do so created the runtime error you've run into. So we should Improve it.
See this example in Athenut mint
When the Mint is created it first checked the passed custom derivation paths to see if one is included for that unit, if it is not is trys to get one of the standard paths, if it is a custom unit it will panic at this point. See here
@davidcaseria and I discussed using the hashing method you bring up and it maybe a good option, I haven't come up with a better one hence this ugly user defined method. The current derivation paths used for the "standard" units isn't actually defined in spec i just copied them from nutshell. I'm not sure if its something that should be defined or left to implementations, worth further discussion.
| @@ -374,45 +374,69 @@ pub enum CurrencyUnit { | |||
| /// Euro | |||
| Eur, | |||
| /// Custom currency unit | |||
| Custom(String), | |||
| Custom(String, u32), | |||
There was a problem hiding this comment.
This would lead to the derivation path being included in the token I believe.
There was a problem hiding this comment.
Do you mean in the ecash token? Is this a problem?
edit: guessing it's a problem because it's not in the spec so other mints using other implementations will not know how to handle these tokens
There was a problem hiding this comment.
Since the serialize/deserialize implementation just use to/from string internally and this modifies those to include or expect the derivation path when the token (cashuB...) get serialized/desrialized it would include or expect the path.
| // Split on ':' to check for derivation index | ||
| let parts: Vec<&str> = value.split(':').collect(); | ||
| let currency = parts[0].to_uppercase(); | ||
|
|
||
| match currency.as_str() { |
There was a problem hiding this comment.
We can't assume that we get the unit in this form it is not in the spec.
if you specify an index over 2^31-1 you get an error from bip32.rs this change throws the error when creating CurrencyUnit it also adds more test cases and cleans up some test code
|
Thanks for the feedback. I'll code up the hash-based solution to see what it looks like. |
Description
You can't actually start a mint using a custom currency unit. It panics with
Error:UnsupportedUnit. The failure starts inMint::newwhen it retrieves aNonederivation path.This PR attempts to fix the issue by requiring the derivation path to be specified when
CurrencyUnit::Customis instantiated. Any u32 derivation path will be accepted except 0-3 which are already allocated to the 4 original currencies.When converting
CurrencyUnitto a string and back it appends the derivation path after the currency string with a colon like in this example:ZUKBUX:12345. There is probably room to improve this. For example we could add a new function to differentiate the full string from just the currency string. I'm open to ideas.I added unit tests for the new functionality.
Notes to the reviewers
I considered creating a deterministic derivation path by hashing the currency string. This could be cool but it would be inflexible and non-standard. I went with a user-defined approach instead.
Suggested CHANGELOG Updates
CHANGED
CurrencyUnit::Customnow requires a derivation path index u32CurrencyUnit::derivation_index()now returnsu32instead ofOption<u32>Displaynow includes a colon and the derivation path index at the end of the stringFromStrexpects to read the derivation path index in the same formatADDED
tests for new functionality
REMOVED
FIXED
Mint::new()no longer fails if you specify a custom currencyChecklist
just final-checkbefore committing