Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Message execution events #66

Merged
merged 16 commits into from
Oct 26, 2023
Merged

Message execution events #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
436d3e1
update message execution events
Oct 24, 2023
bb363ec
update unit tests with new event names
Oct 24, 2023
a193057
fix typos
Oct 24, 2023
7d246ed
update abi bindings
Oct 24, 2023
0c6f072
check message execution success
Oct 24, 2023
45f2245
add isRetry to MessageExecuted event
Oct 25, 2023
78830f2
update abi bindings
Oct 26, 2023
e01bc9b
Merge remote-tracking branch 'origin/main' into execution-events
Oct 26, 2023
6f32342
Revert "add isRetry to MessageExecuted event"
Oct 26, 2023
d042031
update go work sum
Oct 26, 2023
32bc367
Update contracts/src/Teleporter/ITeleporterMessenger.sol
Oct 26, 2023
b725947
remove old doc
Oct 26, 2023
b5c2bca
update bindings
Oct 26, 2023
4d2c0af
add return after storing failed
Oct 26, 2023
4af5c89
Merge remote-tracking branch 'origin/main' into execution-events
Oct 26, 2023
535a8f3
update bindings
Oct 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 54 additions & 54 deletions abi-bindings/go/Teleporter/TeleporterMessenger/TeleporterMessenger.go
View file
Open in desktop

Large diffs are not rendered by default.

12 changes: 7 additions & 5 deletions contracts/src/Teleporter/ITeleporterMessenger.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,18 +62,20 @@ interface ITeleporterMessenger {
* @dev Emitted when Teleporter message is being delivered on destination chain and address,
* but message execution fails. Failed messages can then be retried.
*/
event FailedMessageExecution(
event MessageExecutionFailed(
bytes32 indexed originChainID,
uint256 indexed messageID,
TeleporterMessage message
);

/**
* @dev Emitted when a Teleporter message that previously failed to be executed properly
* is retried and is successfully executed. The message is then no longer able to be retried again
* in the future since each message will be successfully executed at most once.
* @dev Emitted when a Teleporter message is successfully executed with the
* specified destination address and message call data. This can occur either when
* the message is initially received, or on a retry attempt.
*
* Each message received can be executed successfully at most once.
*/
event MessageExecutionRetried(
event MessageExecuted(
bytes32 indexed originChainID,
uint256 indexed messageID
);
Expand Down
11 changes: 8 additions & 3 deletions contracts/src/Teleporter/TeleporterMessenger.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,7 @@ contract TeleporterMessenger is ITeleporterMessenger, ReentrancyGuards {
* @dev See {ITeleporterMessenger-retryMessageExecution}
*
* Reverts if the message execution fails again on specified message.
* Emits a {MessageExecuted} event if the retry is successful.
* Requirements:
*
* - `message` must have previously failed to execute, and matches the hash of the failed message.
Expand Down Expand Up @@ -387,7 +388,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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added isRetry to the MessageExecuted event to differentiate

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think I agree with Cam here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing this up

delete receivedFailedMessageHashes[originChainID][message.messageID];

// Reattempt the message execution with all of the gas left available for execution of this transaction.
Expand Down Expand Up @@ -701,7 +702,8 @@ contract TeleporterMessenger is ITeleporterMessenger, ReentrancyGuards {
* (including possibly storing a failed message in state). All execution specific errors (i.e. invalid call data, etc)
* that are not in the relayers control are caught and handled properly.
*
* Emits a {FailedMessageExecution} event if the call on destination address fails with formatted call data.
* Emits a {MessageExecuted} event if the call on destination address is successful.
* Emits a {MessageExecutionFailed} event if the call on destination address fails with formatted call data.
* Requirements:
*
* - There is enough gas left to cover `message.requiredGasLimit`.
Expand Down Expand Up @@ -749,7 +751,10 @@ contract TeleporterMessenger is ITeleporterMessenger, ReentrancyGuards {
// provided enough gas to meet the required gas limit.
if (!success) {
_storeFailedMessageExecution(originChainID, message);
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch

}

emit MessageExecuted(originChainID, message.messageID);
}

/**
Expand All @@ -765,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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@michaelkaplan13 michaelkaplan13 Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

emit MessageExecutionFailed(originChainID, message.messageID, message);
}

/**
Expand Down
11 changes: 8 additions & 3 deletions contracts/src/Teleporter/tests/HandleInitialMessageExecutionTests.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,12 @@ contract HandleInitialMessageExecutionTest is TeleporterMessengerTest {
// Mock the call to the warp precompile to get the message.
_setUpSuccessGetVerifiedWarpMessageMock(0, warpMessage);

// Receive the message.
// Receive the message and check that message execution was successful.
vm.expectEmit(true, true, true, true, address(teleporterMessenger));
emit MessageExecuted(
DEFAULT_ORIGIN_CHAIN_ID,
messageToReceive.messageID
);
vm.expectEmit(true, true, true, true, address(teleporterMessenger));
emit ReceiveCrossChainMessage(
warpMessage.sourceChainID,
Expand Down Expand Up @@ -217,7 +222,7 @@ contract HandleInitialMessageExecutionTest is TeleporterMessengerTest {
// is considered a failed message execution, but the message itself is
// still successfully delivered.
vm.expectEmit(true, true, true, true, address(teleporterMessenger));
emit FailedMessageExecution(
emit MessageExecutionFailed(
DEFAULT_ORIGIN_CHAIN_ID,
messageToReceive.messageID,
messageToReceive
Expand Down Expand Up @@ -280,7 +285,7 @@ contract HandleInitialMessageExecutionTest is TeleporterMessengerTest {

// Receive the message.
vm.expectEmit(true, true, true, true, address(teleporterMessenger));
emit FailedMessageExecution(
emit MessageExecutionFailed(
DEFAULT_ORIGIN_CHAIN_ID,
messageToReceive.messageID,
messageToReceive
Expand Down
4 changes: 2 additions & 2 deletions contracts/src/Teleporter/tests/RetryMessageExecutionTests.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ contract RetryMessageExecutionTest is TeleporterMessengerTest {
// The message should be successfully received, but its execution should fail.
vm.roll(12);
vm.expectEmit(true, true, true, true, address(teleporterMessenger));
emit FailedMessageExecution(
emit MessageExecutionFailed(
DEFAULT_ORIGIN_CHAIN_ID,
messageToReceive.messageID,
messageToReceive
Expand Down Expand Up @@ -285,7 +285,7 @@ contract RetryMessageExecutionTest is TeleporterMessengerTest {
// Now retry the message execution in a block with an odd height, which should succeed.
vm.roll(13);
vm.expectEmit(true, true, true, true, address(teleporterMessenger));
emit MessageExecutionRetried(originChainID, message.messageID);
emit MessageExecuted(originChainID, message.messageID);
teleporterMessenger.retryMessageExecution(originChainID, message);

// Check that the message had the proper affect on the destination contract.
Expand Down
6 changes: 3 additions & 3 deletions contracts/src/Teleporter/tests/TeleporterMessengerTest.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,13 @@ contract TeleporterMessengerTest is Test {
TeleporterMessage message
);

event FailedMessageExecution(
event MessageExecutionFailed(
bytes32 indexed originChainID,
uint256 indexed messageID,
TeleporterMessage message
);

event MessageExecutionRetried(
event MessageExecuted(
bytes32 indexed originChainID,
uint256 indexed messageID
);
Expand Down Expand Up @@ -240,7 +240,7 @@ contract TeleporterMessengerTest is Test {

// Receive the message - which should fail execution.
vm.expectEmit(true, true, true, true, address(teleporterMessenger));
emit FailedMessageExecution(
emit MessageExecutionFailed(
DEFAULT_ORIGIN_CHAIN_ID,
messageToReceive.messageID,
messageToReceive
Expand Down
3 changes: 3 additions & 0 deletions go.work.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -950,6 +950,7 @@ golang.org/x/net v0.7.0/go.mod h1:2Tu9+aMcznHK/AK1HMvgo6xiTLG5rD5rZLDS+rp2Bjs=
golang.org/x/net v0.9.0/go.mod h1:d48xBJpPfHeWQsugry2m+kC02ZBRGRgulfHnEXEuWns=
golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg=
golang.org/x/net v0.12.0/go.mod h1:zEVYFnQC7m/vmpQFELhcD1EWkZlX69l4oqgmer6hfKA=
golang.org/x/net v0.14.0/go.mod h1:PpSgVXXLK0OxS0F31C1/tv6XNguvCrnXIDrFMspZIUI=
golang.org/x/oauth2 v0.0.0-20221014153046-6fdb5e3db783/go.mod h1:h4gKUeWbJ4rQPri7E0u6Gs4e9Ri2zaLxzw5DI5XGrYg=
golang.org/x/oauth2 v0.5.0/go.mod h1:9/XBHVqLaWO3/BRHs5jbpYCnOZVjj5V0ndyaAM7KB4I=
golang.org/x/oauth2 v0.6.0/go.mod h1:ycmewcwgD4Rpr3eZJLSB4Kyyljb3qDh40vJ8STE5HKw=
Expand All @@ -971,10 +972,12 @@ golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.7.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.10.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.11.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.12.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/text v0.5.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8=
golang.org/x/text v0.7.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8=
golang.org/x/text v0.9.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8=
golang.org/x/text v0.11.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE=
golang.org/x/text v0.12.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE=
golang.org/x/time v0.0.0-20220922220347-f3bd1da661af/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
golang.org/x/tools v0.0.0-20180221164845-07fd8470d635/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
golang.org/x/tools v0.0.0-20190907020128-2ca718005c18/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
Expand Down