The semantics of specifying multiple fees in configuration should be OR instead of AND

[zmanian]

The intended behavior in the mempool is that any denom specified in the configuration parameter --minimum should be sufficient to pay fees. The current behavior requires all denoms to pay fees.

---

For Admin Use:

• Added appropriate labels to PR (ex. wip, ready-for-review, docs)
• Reviewers Assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[codecov]

Codecov Report

Merging #3239 into develop will decrease coverage by 0.02%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #3239      +/-   ##
===========================================
- Coverage    54.88%   54.85%   -0.03%     
===========================================
  Files          133      133              
  Lines         9555     9556       +1     
===========================================
- Hits          5244     5242       -2     
- Misses        3989     3992       +3     
  Partials       322      322

[zmanian]

@alessio can you review and maybe help me write a test for this?

[zmanian]

I have a branch over here the backports this fix to v0.29.* might be helpful for game of stakes.

https://github.com/zmanian/cosmos-sdk/tree/zaki/any_fees_hotfix

[alexanderbez]

Assuming this is what was intended (OR instead of AND), then it LGTM. We should have an CLI and ante handler test(s).

[alexanderbez]

You can update TestGaiaCLIMinimumFees. I'd set --minimum_fees to two types stake and something else (e.g. 1stake,1photino). Then have three test cases:

1. sending a tx with stake as the fee (should succeed)
2. sending a tx with the other denom as the fee (should succeed)
3. sending tx with some other denom (e.g. footoken) (should fail)

[jackzampolin]

I'll take a crack at that @alexanderbez

[jackzampolin]

So I've added the requested cases to the TestGaiaCLIMinimumFees. I'm running into issues. The test as written now sets --minimum_fees=2stake,2feetoken, however sending transactions with a 2stake fee results in the following error message

ERROR: {"codespace":"sdk","code":14,"message":"insufficient fee, got: \"2stake\" required: \"22feetoken,22stake\""}

Note: I've tried different amounts for the min fees and the amount is getting 20 added to it no matter what amount is passed.

Also, while playing around with this I also was able to get a transaction to go through using 2stake for fees when --min_fees=2feetoken. Weird. Anyone have an idea why I'm running into this?

[1xCL]

I've tested zaki's hotfix.
gaiacli tx stake delegate worked with a setting --minimum_fees=1STAKE,1photinos
However, when I use --generate-only flag with the above cmd and manually sign it then broadcast it, I ran in to the same error.

ERROR: {"codespace":"sdk","code":14,"message":"insufficient fee, got: \"1photinos\" required: \"21STAKE,21phtinos\""}

gaiacli tx stake delegate --generate-only > unsigned.tx
gaiacli tx sign unsigned.tx > signed.tx
gaiacli tx broadcast signed.tx

[alexanderbez]

@jackzampolin do you have all the information you need? Perhaps we should hold off on the unit test.

[alexanderbez]

You need to specify the fee even during generate only @winslyn

[jackzampolin]

@alexanderbez I think we need to get the unit test working as right now it isn't

[alessio]

The thing is, this does not check whether a denom is in the coinset B

[alessio]

...and that is cause SafeMinus does not carry out such check

[alexanderbez]

Thanks @alessio -- just a minor bit of feedback.

[alexanderbez]

Pending approval once integration_tests is fixed 👍