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

Jump to bottom

Signalling #89

Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open

Signalling #89

wants to merge 36 commits into from

Conversation

LeoPatOZ
Copy link
Collaborator

@LeoPatOZ LeoPatOZ commented Mar 27, 2025 •
edited
Loading

This PR aims at adding additional signalling functionality.

  1. Adding a canonical ETH bridge
  2. Simplifying the signal service
  3. Added historical commitment tracking

QUESTIONS:

Checkpoint Tracker

  • How often should we store the commitments, based off timestamp or no. commitments?

ggonzalez94
ggonzalez94 reviewed Mar 28, 2025
View reviewed changes
Copy link
Collaborator

@ggonzalez94 ggonzalez94 left a comment

Choose a reason for hiding this comment

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

Just did a first pass on the signaling mechanism, will continue with the CheckpointSyncer and briding. The most important feedback so far is that we are missing some parameters when verifying the signals

src/libs/LibSignal.sol
/// @dev Signal a `value` at a namespaced slot. See `deriveSlot`.
function signal(uint64 chainId, address account, bytes32 value) internal returns (bytes32) {
bytes32 slot = deriveSlot(chainId, account, value);
slot.getBytes32Slot().value = value;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we already have the storage slot, is there any gas savings from doing this directly with assembly? I'm not familiar with the implementation of getBytes32Slot, I'll check it out - this is just not to miss the idea

assembly {
      sstore(slot, value)
  }

LeoPatOZ and others added 3 commits March 28, 2025 19:54
@LeoPatOZ @ggonzalez94
Update src/protocol/ISignalService.sol
d8440ab 
Co-authored-by: Gustavo Gonzalez <gustavo.gonzalez@openzeppelin.com>
@LeoPatOZ @ggonzalez94
Update src/protocol/ISignalService.sol
1d2d6c1 
Co-authored-by: Gustavo Gonzalez <gustavo.gonzalez@openzeppelin.com>
@LeoPatOZ
pr comments
ba82fcc
@LeoPatOZ
remove root as a param
d7da73f 
simplify

comment

updated event

explain root
@LeoPatOZ
root -> proof
96c4b94 
re-do order

comment
ggonzalez94
ggonzalez94 reviewed Mar 31, 2025
View reviewed changes
Copy link
Collaborator

@ggonzalez94 ggonzalez94 left a comment

Choose a reason for hiding this comment

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

I know this is still in draft, but I like how it is looking, much cleaner!
One question: Do you think it is worth implementing the ring buffer mechanism now or do you prefer to implement that later?

src/protocol/CheckpointSyncer.sol Outdated
abstract contract CheckpointSyncer is ICheckpointSyncer {
address private _trustedSyncer;

mapping(uint256 height => bytes32 checkpoint) private _checkpoints;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use the term checkpoint to represent a struct with the publicationId and the commitment(state root or block hash). I think it might be confusing if we are also naming this checkpoints(which is a subset of the actual checkpoint). Not sure if this is the best, bust perhaps:

Suggested change
mapping(uint256 height => bytes32 checkpoint) private _checkpoints;
mapping(uint256 height => bytes32 commitment) private _commitments;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah i was in between the two, its just in my mind a commitment is an unproven checkpoint and this only tracks "proven checkpoints" aka block hash / state root but yes confusing with our custom checkpoint struct

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed everything to reflect commitment, also changed contract to commitment store as syncing is probably the wrong verb

@LeoPatOZ
refactored to reflect commtiment
0ed24b6 
comment

better error
@LeoPatOZ
Copy link
Collaborator Author

LeoPatOZ commented Apr 1, 2025

I know this is still in draft, but I like how it is looking, much cleaner! One question: Do you think it is worth implementing the ring buffer mechanism now or do you prefer to implement that later?

added the ring buffer now

@LeoPatOZ
added ring buffer
41b0f2a 
comment

typo
@LeoPatOZ LeoPatOZ changed the base branch from main to delayed-binary-search April 2, 2025 11:25
@LeoPatOZ LeoPatOZ changed the base branch from delayed-binary-search to main April 2, 2025 11:26
@LeoPatOZ LeoPatOZ marked this pull request as ready for review April 2, 2025 13:34
@LeoPatOZ
custom error
45fb510 
comments

comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggonzalez94 ggonzalez94 ggonzalez94 left review comments

@nikeshnazareth nikeshnazareth Awaiting requested review from nikeshnazareth

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LeoPatOZ @ggonzalez94 @nikeshnazareth