audit(erc20): state machine audit

[danburck]

Description

This PR adds minor improvements from performing a state machine audit on the x/erc20 module:

• improved comments
• moved evm relevant helper methods to evm.go
• updated specs

Open questions:

• are the migrations necessary?

Future improvements

• Toggle token pair: Should we revert the token transfer through the hook if a token pair is disabled?
• Add tests make sure that if ConvertERC20 is called, that the Hook doesn't trigger

[linear]

ENG-218 x/erc20 Readiness - State machine audit

[codecov]

Codecov Report

Merging #547 (8f91db5) into main (4da7c8d) will decrease coverage by 0.06%.
The diff coverage is 77.85%.

❗ Current head 8f91db5 differs from pull request most recent head 2ea056b. Consider uploading reports for the commit 2ea056b to get more accurate results

@@            Coverage Diff             @@
##             main     #547      +/-   ##
==========================================
- Coverage   81.64%   81.57%   -0.07%     
==========================================
  Files         115      115              
  Lines        6275     6318      +43     
==========================================
+ Hits         5123     5154      +31     
- Misses        997     1009      +12     
  Partials      155      155              

Impacted Files	Coverage Δ
x/erc20/keeper/evm.go	67.97% <66.66%> (-0.30%)	⬇️
x/erc20/keeper/msg_server.go	77.94% <66.66%> (+0.04%)	⬆️
x/erc20/keeper/proposals.go	87.67% <83.01%> (+1.67%)	⬆️
x/erc20/keeper/mint.go	87.17% <85.00%> (-1.29%)	⬇️
x/erc20/keeper/evm_hooks.go	88.60% <100.00%> (-0.29%)	⬇️

[fedekunze]

are the migrations necessary?

No, because we don't need to migrate the state

Toggle token pair: Should we revert the token transfer through the hook if a token pair is disabled?

I think just disabling the transfers is enough. If we reverted then it would disable and affect all ERC20 transfers for that contract

[fedekunze]

LGTM 💯