Deduct min fee on error improvements and tests #364
Conversation
husio
changed the title
x/cash/dynamicfee_test.go
Outdated
| minumumFee: x.NewCoin(0, 23, ""), | ||
| txFee: x.NewCoin(0, 421, "ETH"), | ||
| wantCheckErr: errors.ErrInsufficientAmount, | ||
| wantCheckTxFee: x.NewCoin(0, 23, "BTC"), |
There was a problem hiding this comment.
note: that's quite an expensive operation, now. (in terms of money). We better have tickers set. :-)
x/cash/dynamicfee_test.go
Outdated
| if want.IsZero() { | ||
| // This is a weird case when a transaction was | ||
| // submitted but the signed does not have | ||
| // enough funds to pay the minumum (anty spam) |
There was a problem hiding this comment.
note: the account does not exist so the signer never had any coins.
There was a problem hiding this comment.
I just went though the comments.
I want to clarify that a non-zero minimum fee without a ticker is a meaningless concept and should not be supported (explicitly return error as soon as detected). In fact gconf should not allow setting such coins in configuration, as they are invalid. (if !coin.IsZero() { return coin.Validate() } could be a good check in gconf parsing, as well as in the execution stack of enforcing such fee
There was a problem hiding this comment.
Nice work and nice test cases.
I would just like two minor cleanups addressed...
The shadowing of err in Check/Deliver on the decorator, which crucially depend on which err is referenced in the defer block. Let's make this more explicit.
Also, the embedding of the nil weave.Handler in mockHandler should be simplified. Integrating this into x.TestHelpers can be another PR, but a nice cleanup to make reusable helpers
| cache.Write() | ||
| res.GasPayment += toPayment(fee) | ||
| return res, err | ||
| if err := d.chargeFee(cache, payer, fee); err != nil { |
There was a problem hiding this comment.
I am very unsure of how all this relates to the returned error. Along with scoping.
I mean, we have err as a return value
And this line defines a local err
Then when we return below, defer is executed and gets the errors.Wrap(...) error, right?
I mean, on one hand it looks clean, on the other, this is depending quite a bit on the details of scoping. I would rather avoid shadowing if possible. I mean if the return value was eg. rerr for return error, then it is clear if we are refering to the function-local err or the return value one. In general, I would limit shadowing and redefinition of variables as it makes it easy for subtle bugs to drop in.
I think this code is correct, but would prefer to remove this crucial shadow that affects the defer() function
There was a problem hiding this comment.
I was wondering the same thing. Tests are covering those cases and they seem to work as expected.
I do not know how to write it differently. I could define two anonymous functions or use goto. I think they all suffer the same issue.
There was a problem hiding this comment.
I changed the name of the returned values to ferr and fres (final error, final result), so they are not used at all in the normal code, just references for the defer function to use. Exact same logic and control flow you have, I just renamed one variable to avoid shadowing. I would consider adding an anti-shadowing lint rule.. or maybe I just got used to it from the TypeScript linting I was using in another project, where it is not allowed and prevents a number of gotchas.
x/cash/dynamicfee.go
Outdated
| if cmp.Ticker == "" { | ||
| cmp.Ticker = fee.Ticker | ||
| minFee := gconf.Coin(store, GconfMinimalFee) | ||
| // If the minimum fee has no currency accept anything. |
There was a problem hiding this comment.
Yes, we will address that in another issue #369
I wonder if this works as expected 🤔
Tests will show once they will arrive 🏎️