Message execution events #66

minghinmatthewlam · 2023-10-24T21:06:35Z

Why this should be merged

Clean up of message execution events, consolidating event for message execution success and failure.

How this works

Previously we just had an event for successful retry MessageExecutionRetried and execution failure FailedMessageExecution, but not an event emitted for successful execution. We now emit MessageExecuted for successful initial or retry execution, and MessageExecutionFailed when execution fails.

How this was tested

unit tests

How is this documented

updated natspec comments

geoff-vball

👍

bernard-avalabs

LGTM

cam-schultz · 2023-10-25T14:39:46Z

contracts/src/Teleporter/TeleporterMessenger.sol

@@ -388,7 +389,7 @@ contract TeleporterMessenger is ITeleporterMessenger, ReentrancyGuards {

        // Clear the failed message hash from state prior to retrying its execution to redundantly prevent
        // reentrance attacks (on top of the nonReentrant guard).
-        emit MessageExecutionRetried(originChainID, message.messageID);
+        emit MessageExecuted(originChainID, message.messageID);


Won't affect correctness since we revert on execution failure, but can we move this to after the call itself and the associated require?

It might also be worth keeping MessageExecutionRetried so that an indexer can distinguish between calls to retryMessageExecution and receiveCrossChainMessage based solely on the logs.

I think if we want to distinguish these two, it might be better for have a bool flag in MessageExecuted instead. I think it would be good to have all executed messages emit a MessageExecuted event, so they can all be easily grouped together.

Yup, agreed that all executed messages should emit MessageExecuted. No strong preference if the distinction between retryMessageExecution and receiveCrossChainMessage is done with a bool flag or a separate MessageExecutionRetried event, but I think the distinction should be made one way or another.

Ah, yeah emitting two events is probably a little cleaner, and should be okay since retries shouldn't be too common

added isRetry to the MessageExecuted event to differentiate

Actually, I have a preference for separate retry vs executed events over the boolean flag approach.

If we expect retries to be relatively rare, then it doesn't make as much sense for every MessageExecuted event to pay gas for the indexed isRetry field.

OTOH, if retries are common enough, then it's possible for the average gas needed to eventually execute the message would be lower by only having a single event type.

I suspect the former case to be more likely, so would prefer separate events over the bool flag.

Yeah I think I agree with Cam here

We synced offline and decided that it makes the most sense to not distinguish between the initial execution and retry cases, and have retryMessageExecution emit only MessageExecuted without the bool flag. The main reason for this is gas savings by not emitting more events than are necessary to reconstruct on-chain activity. An indexer should be able to construct all the scenarios with the existing events associated with a particular message ID:

(Happy case) initial execution succeeds - observe MessageExecuted

Initial execution fails - observe MessageExecutionFailed

Successful retry - observe MessageExecutionFailed followed by MessageExecuted in a later tx

Failed retry - observe MessageExecutionFailed followed by a tx revert on the failed call to retryMessageExecution

Thanks for writing this up

michaelkaplan13

Just one nit on comment wording, but otherwise LGTM

contracts/src/Teleporter/ITeleporterMessenger.sol

This reverts commit 45f2245.

Co-authored-by: Michael Kaplan <[email protected]> Signed-off-by: minghinmatthewlam <[email protected]>

cam-schultz · 2023-10-26T20:27:27Z

contracts/src/Teleporter/TeleporterMessenger.sol

@@ -766,7 +770,7 @@ contract TeleporterMessenger is ITeleporterMessenger, ReentrancyGuards {
        ] = keccak256(abi.encode(message));

        // Emit a failed execution event for anyone monitoring unsuccessful messages to retry.
-        emit FailedMessageExecution(originChainID, message.messageID, message);


Do we want to add the failure reason to this log?

IMO, that is best left for consideration at a later point in time because we're not sure what the return data will contain in various failure cases. Up to @minghinmatthewlam either way though

Sounds good. I created an issue here: #81. @minghinmatthewlam up to you if you want to include that in this PR.

Thanks will tackle in separate PR, definitely think will be helpful for developers for debugging so we should add.

cam-schultz · 2023-10-26T21:25:16Z

contracts/src/Teleporter/TeleporterMessenger.sol

@@ -752,6 +752,7 @@ contract TeleporterMessenger is ITeleporterMessenger, ReentrancyGuards {
        // provided enough gas to meet the required gas limit.
        if (!success) {
            _storeFailedMessageExecution(originChainID, message);
+            return;


geoff-vball

👍

Matthew Lam added 5 commits October 24, 2023 19:52

update message execution events

436d3e1

update unit tests with new event names

bb363ec

fix typos

a193057

update abi bindings

7d246ed

check message execution success

0c6f072

minghinmatthewlam requested review from michaelkaplan13, cam-schultz, gwen917, geoff-vball and bernard-avalabs as code owners October 24, 2023 21:06

geoff-vball previously approved these changes Oct 25, 2023

View reviewed changes

bernard-avalabs previously approved these changes Oct 25, 2023

View reviewed changes

cam-schultz reviewed Oct 25, 2023

View reviewed changes

add isRetry to MessageExecuted event

45f2245

minghinmatthewlam dismissed stale reviews from bernard-avalabs and geoff-vball via 45f2245 October 25, 2023 23:58

minghinmatthewlam requested review from geoff-vball, cam-schultz and bernard-avalabs October 25, 2023 23:59

update abi bindings

78830f2

michaelkaplan13 reviewed Oct 26, 2023

View reviewed changes

contracts/src/Teleporter/ITeleporterMessenger.sol Outdated Show resolved Hide resolved

Matthew Lam and others added 6 commits October 26, 2023 20:18

Merge remote-tracking branch 'origin/main' into execution-events

e01bc9b

Revert "add isRetry to MessageExecuted event"

6f32342

This reverts commit 45f2245.

update go work sum

d042031

Update contracts/src/Teleporter/ITeleporterMessenger.sol

32bc367

Co-authored-by: Michael Kaplan <[email protected]> Signed-off-by: minghinmatthewlam <[email protected]>

remove old doc

b725947

update bindings

b5c2bca

minghinmatthewlam requested a review from michaelkaplan13 October 26, 2023 20:24

cam-schultz reviewed Oct 26, 2023

View reviewed changes

michaelkaplan13 previously approved these changes Oct 26, 2023

View reviewed changes

cam-schultz previously approved these changes Oct 26, 2023

View reviewed changes

add return after storing failed

4d2c0af

minghinmatthewlam dismissed stale reviews from cam-schultz and michaelkaplan13 via 4d2c0af October 26, 2023 21:07

minghinmatthewlam requested review from michaelkaplan13 and cam-schultz October 26, 2023 21:08

michaelkaplan13 previously approved these changes Oct 26, 2023

View reviewed changes

Matthew Lam added 2 commits October 26, 2023 21:21

Merge remote-tracking branch 'origin/main' into execution-events

4af5c89

update bindings

535a8f3

minghinmatthewlam dismissed michaelkaplan13’s stale review via 535a8f3 October 26, 2023 21:22

cam-schultz approved these changes Oct 26, 2023

View reviewed changes

geoff-vball approved these changes Oct 26, 2023

View reviewed changes

minghinmatthewlam merged commit 28def26 into main Oct 26, 2023
9 of 11 checks passed

geoff-vball deleted the execution-events branch October 26, 2023 22:41

minghinmatthewlam pushed a commit that referenced this pull request Oct 1, 2024

Required gas, events, reentrancy, go linter (#66)

a211330

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Message execution events #66

Message execution events #66

minghinmatthewlam commented Oct 24, 2023

geoff-vball left a comment

bernard-avalabs left a comment

cam-schultz Oct 25, 2023

cam-schultz Oct 25, 2023

geoff-vball Oct 25, 2023

cam-schultz Oct 25, 2023

geoff-vball Oct 25, 2023

minghinmatthewlam Oct 25, 2023

cam-schultz Oct 26, 2023

geoff-vball Oct 26, 2023

cam-schultz Oct 26, 2023

minghinmatthewlam Oct 26, 2023

michaelkaplan13 left a comment

cam-schultz Oct 26, 2023

michaelkaplan13 Oct 26, 2023 •

edited

Loading

cam-schultz Oct 26, 2023

minghinmatthewlam Oct 26, 2023

cam-schultz Oct 26, 2023

geoff-vball left a comment

Message execution events #66

Message execution events #66

Conversation

minghinmatthewlam commented Oct 24, 2023

Why this should be merged

How this works

How this was tested

How is this documented

geoff-vball left a comment

Choose a reason for hiding this comment

bernard-avalabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkaplan13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkaplan13 Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoff-vball left a comment

Choose a reason for hiding this comment

michaelkaplan13 Oct 26, 2023 •

edited

Loading