Minimal fee requires a ticker #385
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Code is fine.
One question about gconf... There is no way to return an error on parsing init if there is no ticker on the fee, but it will cause every tx on the chain to error.
I looked at gconf/genesis.go and it doens't know the type at all on parsing, which makes this impossible in current design.
Thus, nothing simple to do here as part of this PR, but a thought for future enhancements... to add some validation steps on init... Maybe Initializer of cash could run after gconf Initializer, then load those gconf values and do custom assertions on it (like IsZero() or Valdiate() passes).
What do you think? Stuff for a different issue in any case....
| if cmp.Ticker == "" { | ||
| cmp.Ticker = fee.Ticker | ||
| return nil, errors.Wrap(coin.ErrInvalidCurrency, "no ticker") |
There was a problem hiding this comment.
This is fine, but isn't this also covered by the !fee.SameType() line below?
There was a problem hiding this comment.
True. Look like Coin.IsGTE is calling SameType as well, which means it would cover all three cases. I think I prefer verbosity here. Having those 3 rules tested explicitly made it easier for me to understand what is expected from the data in order to be valid.
I will merge it as it is right now. If you think this should be changed then I can create another pull request.
| &FeeInfo{Fees: &min}, | ||
| min, | ||
| errors.ErrEmpty.Is, | ||
| "no fee given, nothing expected": { |
There was a problem hiding this comment.
thanks for the cleanup.
one i found your map-driven (not table driven) tests with labels, there is no going back. love the descriptive names in the test errors (not test-4)
There was a problem hiding this comment.
I have learned it from one of the meetups in Berlin https://youtu.be/yszygk1cpEc?t=712
|
by the way, i like reading sounds like something yoda would say |
Do not allow a minimal fee to not have a ticker.
resolve #369