Skip to content

feat: L2ToL2CDM SentMessage originContext#15844

Closed
hamdiallam wants to merge 3 commits intodevelopfrom
05-12-sent_message_ctx_l2tol2cdm.sentmessage.ctx
Closed

feat: L2ToL2CDM SentMessage originContext#15844
hamdiallam wants to merge 3 commits intodevelopfrom
05-12-sent_message_ctx_l2tol2cdm.sentmessage.ctx

Conversation

@hamdiallam
Copy link
Copy Markdown
Contributor

@hamdiallam hamdiallam commented May 12, 2025

Implementation of Design Doc.

hamdiallam added 2 commits May 12, 2025 11:40
@hamdiallam
sent message ctx
e71fb18 
l2tol2cdm.sentmessage.ctx
@hamdiallam
pre-pr
d507b00
@hamdiallam hamdiallam requested a review from a team as a code owner May 12, 2025 15:48
@hamdiallam hamdiallam requested a review from AmadiMichael May 12, 2025 15:48
@hamdiallam Graphite App
Copy link
Copy Markdown
Contributor Author

hamdiallam commented May 12, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 12, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.99%. Comparing base (23c1fef) to head (b22f083).
Report is 129 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #15844      +/-   ##
===========================================
- Coverage    46.86%   45.99%   -0.87%     
===========================================
  Files         1338     1283      -55     
  Lines       108127   103891    -4236     
===========================================
- Hits         50672    47788    -2884     
+ Misses       53847    52630    -1217     
+ Partials      3608     3473     -135
Flag Coverage Δ
cannon-go-tests-64 ?
contracts-bedrock-tests 96.06% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...acts-bedrock/src/L2/L2ToL2CrossDomainMessenger.sol 100.00% <100.00%> (ø)
...ckages/contracts-bedrock/src/libraries/Hashing.sol 95.83% <100.00%> (ø)

... and 62 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

hamdiallam
hamdiallam commented May 12, 2025
View reviewed changes
Comment thread packages/contracts-bedrock/src/L2/L2ToL2CrossDomainMessenger.sol
bytes32 internal constant CROSS_DOMAIN_MESSAGE_SOURCE_SLOT =
0x711dfa3259c842fffc17d6e1f1e0fc5927756133a2345ca56b4cb8178589fee7;

/// @notice Event selector for the SentMessage event. Will be removed in favor of reading
Copy link
Copy Markdown
Contributor Author

@hamdiallam hamdiallam May 12, 2025

Choose a reason for hiding this comment

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

no longer needed with the merged bugfix

hamdiallam
hamdiallam commented May 12, 2025
View reviewed changes
Comment thread packages/contracts-bedrock/src/libraries/Hashing.sol
address _target,
bytes memory _message
bytes memory _message,
bytes memory _context
Copy link
Copy Markdown
Contributor Author

@hamdiallam hamdiallam May 12, 2025

Choose a reason for hiding this comment

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

critical to include here so that resendMessage works as intended

tynes
tynes reviewed May 12, 2025
View reviewed changes
Comment thread packages/contracts-bedrock/test/L2/ExecutingMessageEmitted.t.sol
@@ -16,7 +16,7 @@ import { ISuperchainTokenBridge } from "interfaces/L2/ISuperchainTokenBridge.sol
/// @notice Integration test that checks that the `ExecutingMessage` event is emitted on crosschain mints.
contract ExecutingMessageEmittedTest is CommonTest {
bytes32 internal constant SENT_MESSAGE_EVENT_SELECTOR =
Copy link
Copy Markdown
Contributor

@tynes tynes May 12, 2025

Choose a reason for hiding this comment

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

Can we use the selector property on the event itself rather than hardcoding it and needing it to change?

tynes
tynes reviewed May 12, 2025
View reviewed changes
Comment thread packages/contracts-bedrock/src/L2/L2ToL2CrossDomainMessenger.sol
if (_target == Predeploys.L2_TO_L2_CROSS_DOMAIN_MESSENGER) revert MessageTargetL2ToL2CrossDomainMessenger();

uint256 nonce = messageNonce();
bytes memory context = "";
Copy link
Copy Markdown
Contributor

@tynes tynes May 12, 2025

Choose a reason for hiding this comment

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

I am confused, should there be a sendMessage(uint,address,bytes,bytes) in addition where the context can be set?

@tynes
Copy link
Copy Markdown
Contributor

tynes commented May 12, 2025 •
edited
Loading

Can you link the specs PR to this PR?

edit: I see ethereum-optimism/specs#700

@opgitgovernance opgitgovernance added the S-stale Status: Will be closed unless there is activity label May 27, 2025
@opgitgovernance
Copy link
Copy Markdown
Contributor

opgitgovernance commented May 27, 2025

This pr has been automatically marked as stale and will be closed in 5 days if no updates

@hamdiallam hamdiallam changed the title feat: L2ToL2CDM SentMessage ctx field feat: L2ToL2CDM SentMessage originContext May 29, 2025
@hamdiallam
Copy link
Copy Markdown
Contributor Author

hamdiallam commented May 29, 2025

Closing in favor of #16150

@hamdiallam hamdiallam closed this May 29, 2025
@aliersh aliersh deleted the 05-12-sent_message_ctx_l2tol2cdm.sentmessage.ctx branch July 21, 2025 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tynes tynes tynes left review comments

@AmadiMichael AmadiMichael Awaiting requested review from AmadiMichael AmadiMichael is a code owner automatically assigned from ethereum-optimism/contract-reviewers

Assignees

No one assigned

Labels

S-stale Status: Will be closed unless there is activity

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hamdiallam @tynes @opgitgovernance