go/staking: add minimum transfer amount

[pro-wh]

cross reference: https://oasisprotocol.clickup.com/t/1zfn2hb

[Yawning]

I guess this needs fixgenesis support, otherwise it looks fine to me.

[pro-wh]

hm, maybe I should perform the check on withdraw too, for symmetry

[pro-wh]

adding to fixgenesis: the proposal is to set it to 1 base unit. this particular change is mostly to prevent zero-amount transfers from creating useless empty accounts.

[pro-wh]

rearranged checks: now compares with min transfer amount parameter after IsSimulation bail, similar to min delegation amount check.

added similar check to withdraw (same parameter)

[Yawning]

Should this be applied to burn and allow as well?

[kostko]

Should this be applied to burn and allow as well?

I don't think allow is problematic (as it only updates the account's allowance, a zero allowance is ignored and the total number of allowances per account is already limited), the more important thing is withdraw (which seems to be handled in this PR).

[kostko]

For burn we could just ignore burning a zero amount as that results in a useless event to be emitted.

I guess an additional question is whether we would want to require a minimum balance to remain in an account, otherwise operations fail? Otherwise one can just do this at no cost (besides gas fees):

• Transfer min_transfer to A.
• Transfer min_transfer from A to B.
• Transfer min_transfer from B to C.
• ...

[kostko]

This is a breaking change to the consensus protocol.

[pro-wh]

we should do the other transactions as well. it's not so much that allow, for example, won't really charge the allowances table, but moreso that we have to create an account and start tracking the nonce after a successful transaction

[pro-wh]

let's have the minimum balance thing in a separate PR.

[kostko]

we should do the other transactions as well. it's not so much that allow, for example, won't really charge the allowances table, but moreso that we have to create an account and start tracking the nonce after a successful transaction

True, but this basically applies to any successfully authenticated transaction even though it doesn't deal with tokens (besides paying for fees). So this would basically imply a minimum account balance, because the check would need to be "you need to have at least X in your account before you can make any transaction"?

[pro-wh]

yeah, not sure if there's another mitigation for allow

[pro-wh]

added similar check (same parameter) to burn

adjusted ordering of checks in withdraw to check amount before consulting transfers disablement 🤷

[pro-wh]

let's leave allow and others as is and proceed to draft a minimum balance requirement \:

[kostko]

I think this looks good. Just change that feature entry to breaking.

[pro-wh]

we discussed this further yesterday, and I'll be changing the minimum amount

[codecov]

Codecov Report

Merging #4456 (2e9becb) into master (a53d21f) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4456      +/-   ##
==========================================
+ Coverage   68.78%   68.84%   +0.05%     
==========================================
  Files         424      424              
  Lines       47359    47366       +7     
==========================================
+ Hits        32576    32608      +32     
+ Misses      10762    10739      -23     
+ Partials     4021     4019       -2     

Impacted Files	Coverage Δ
go/staking/api/api.go	62.33% <ø> (ø)
.../consensus/tendermint/apps/staking/transactions.go	71.67% <100.00%> (+0.41%)	⬆️
go/staking/tests/state.go	95.39% <100.00%> (+0.02%)	⬆️
go/consensus/tendermint/full/services.go	77.69% <0.00%> (-7.70%)	⬇️
go/worker/beacon/tx_retry.go	90.47% <0.00%> (-4.77%)	⬇️
go/storage/mkvs/insert.go	87.75% <0.00%> (-4.09%)	⬇️
go/worker/beacon/worker_vrf.go	64.34% <0.00%> (-1.74%)	⬇️
...consensus/tendermint/apps/registry/transactions.go	54.60% <0.00%> (-1.72%)	⬇️
.../consensus/tendermint/apps/roothash/state/state.go	73.91% <0.00%> (-1.09%)	⬇️
go/consensus/tendermint/roothash/roothash.go	67.86% <0.00%> (-0.64%)	⬇️
... and 14 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f20e303...2e9becb. Read the comment docs.

[pro-wh]

summary of changes after review:

1. proposed value (via fixgenesis) set to 0.01 ROSE (10_000_000 base units)
2. test fixes
   a. transfer and withdraw result return
   b. TestReservedAddresses use non-nil tx body
3. staking tests GenesisState sets a non-empty MinTransferAmount, so we can exercise its serialization
4. updated TestGenesisChainContext