chore(evmengine): refactor evmmsgs #106
Conversation
Before: the evmEvents function handled - Fetching logs from ethereum client - Converting those logs to EVMEvent objects - Verifying the converted events - Sorting the events After: The evmEvents function - Fetching logs from ethereum client - Delegates conversion and verification to types.EthLogToEVMEvent. - Delegates sorting to types.SortEVMEvents.
zsystm
requested review from
LeoHChen,
AndyBoWu,
jdubpark,
edisonz0718,
ezreal1997,
limengformal,
Narangde and
leeren
as code owners
client/x/evmengine/types/tx_test.go
Outdated
| slashingAbi, err := bindings.IPTokenSlashingMetaData.GetAbi() | ||
| require.NoError(t, err, "failed to load ABI") | ||
| slashingAddr := common.HexToAddress(predeploys.IPTokenSlashing) | ||
| require.Negative(t, bytes.Compare(stakingAddr.Bytes(), slashingAddr.Bytes()), "stakingAddr should be less than slashingAddr") |
There was a problem hiding this comment.
What do you mean stakingAddr should be less than slashingAddr for a compare? (+ same for below error messages)
There was a problem hiding this comment.
@jdubpark
Thanks for the feedback! The message is somewhat unclear and difficult to understand.
I have removed it and added more detailed comments in the code explaining why these comparisons are necessary and included examples to illustrate the expected behavior.
Please take a look at the updated comments, and let me know if there’s anything else I can clarify.
Note:
The reason for checking that stakingAddr is less than slashingAddr is to ensure a known initial order before testing the sorting logic.
This precondition is necessary because we want to verify that if the addresses are input in the wrong order, the sorting function will correctly reorder them.
Similarly, the same check is applied for the event topics (withdrawEv and depositEv) to confirm the function’s behavior.
zsystm
changed the title
client/x/evmengine/types/tx.go
Outdated
| } | ||
|
|
||
| // IsSortedEVMEvents check if the events are sorted by ascending order of address, topics, and data. | ||
| func IsSortedEVMEvents(events []*EVMEvent) bool { |
There was a problem hiding this comment.
Is this only used in the test? If so, we should move it to the test
There was a problem hiding this comment.
There was a problem hiding this comment.
@edisonz0718
Could you review this PR when you have time? I applied your comment. Thanks in advance.
refactored evmmsgs for better code quailty and testability.
issue: #105