Add adr-002 evm hooks proposal

[yihuang]

Description

ref impl: #417

---

For contributor use:

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer

---

For admin use:

• Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
• Reviewers assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[khoslaventures]

This ADR doesn't explain why we need the PostTxProcessing hook. Can you explain in here how it will

enable EVM contract to communicate with cosmos native modules

[tomtau]

perhaps adding some examples from: #394 (comment) ?

[yihuang]

done, added the bank send example.

[orijbot]

Visit https://dashboard.github.orijtech.com?back=0&pr=429&remote=true&repo=yihuang%2Fethermint to see benchmark details.

[codecov]

Codecov Report

Merging #429 (082d60e) into main (516ffe2) will increase coverage by 0.36%.
The diff coverage is n/a.

❗ Current head 082d60e differs from pull request most recent head c38957a. Consider uploading reports for the commit c38957a to get more accurate results

@@            Coverage Diff             @@
##             main     #429      +/-   ##
==========================================
+ Coverage   50.35%   50.71%   +0.36%     
==========================================
  Files          57       49       -8     
  Lines        5459     4874     -585     
==========================================
- Hits         2749     2472     -277     
+ Misses       2594     2297     -297     
+ Partials      116      105      -11     

Impacted Files	Coverage Δ
types/chain_id.go	57.89% <0.00%> (-26.32%)	⬇️
x/evm/types/chain_config.go	76.61% <0.00%> (-20.85%)	⬇️
x/evm/keeper/state_transition.go	58.10% <0.00%> (-16.17%)	⬇️
server/config/config.go	17.85% <0.00%> (-3.42%)	⬇️
x/evm/keeper/statedb.go	84.76% <0.00%> (-1.49%)	⬇️
x/evm/types/utils.go	60.00% <0.00%> (-1.12%)	⬇️
x/evm/genesis.go	44.26% <0.00%> (-0.74%)	⬇️
app/app.go	83.37% <0.00%> (-0.43%)	⬇️
x/evm/keeper/grpc_query.go	70.88% <0.00%> (-0.33%)	⬇️
types/codec.go	0.00% <0.00%> (ø)
... and 17 more

[fedekunze]

The ADR is good, but it doesn't follow the ADR template and a lot of context and details are missing if you compare it with the implementation. For example, the following decision is not documented on the ADR:

if !res.Failed() {
	err = k.PostTxProcessing(txHash, logs)
	if err != nil {
		k.RevertToSnapshot(revision)
		res.VmError = "failed to process native logs"
		res.Ret = []byte(err.Error())
	}
}

Some questions that need to be documented:

• why do we revert the state to the snapshot prior to the tx execution? (i.e why did you choose this approach over just performing a no-op on PostTxProcessing)
• give a reference implementation for a Hook implementation
• explain the MultiHook design for adding multiple hooks
• explain how to add the Hook to the app.go

[yihuang]

The ADR is good, but it doesn't follow the ADR template and a lot of context and details are missing if you compare it with the implementation. For example, the following decision is not documented on the ADR:

if !res.Failed() {
	err = k.PostTxProcessing(txHash, logs)
	if err != nil {
		k.RevertToSnapshot(revision)
		res.VmError = "failed to process native logs"
		res.Ret = []byte(err.Error())
	}
}

added a little bit more explanation to this:

• hooks are called only when tx executed successfully
• errors returned by hooks get translated to vm error, and cause the whole tx to revert.

Some questions that need to be documented:

• why do we revert the state to the snapshot prior to the tx execution? (i.e why did you choose this approach over just performing a no-op on PostTxProcessing)

added, with this approach, the implementor is still free to swallow any error.

• give a reference implementation for a Hook implementation

add the BankSendHook to adr.

• explain the MultiHook design for adding multiple hooks

I think this is not essential for the proposal?

• explain how to add the Hook to the app.go

added.

[fedekunze]

Looks better now, although there are some details and code snippet are missing about the integration with the state_transition.go.

I will also recommend you use Grammarly to fix some of the wording.

[yihuang]

Looks better now, although there are some details and code snippet are missing about the integration with the state_transition.go.

done, added some ApplyTransaction code snippet.

I will also recommend you use Grammarly to fix some of the wording.

done

[fedekunze]

ACK 👍