[2/2] Decouple ETH Bridge from signal service #113

LeoPatOZ · 2025-04-30T11:34:56Z

PR solves:

Generalise the ETH bridge to handle multiple rollups.

height --> index

pure

fix fix test fix test

fix fix test fix test single

The previous comment was ambiguous about whether we support full state roots. I think `StateProofNotSupported` as an error is unclear, and might actually state the opposite of the error. For context - the `LibSignal` library attempts to prove a storage location inside a root. - we expect this to be a state root. The proof is multi-step: first prove the account's storage root within the state proof, then prove the storage location against the storage root. - the library also supports skipping step 1 and treating the root as a storage root directly. The signal service does not. At some point we will have to generalise the commitment and storage proof mechanism to support block hashes as commitments (which is natural for rollups) and arbitrary state for appchains. In the mean time we are being opinionated that the commitment is a state root.

comment comment better error comment

github-actions · 2025-05-06T13:55:09Z

Changes to gas cost

Generated at commit: a389c8da0996b137f208702d5ff0f37f8e2ea5de, compared to commit: a5ad6175f1b4a9790883b7416f07052d5580ee66

🧾 Summary (10% most significant diffs)

Contract	Method	Avg (+/-)	%
SignalService	commitmentAt	-22 ✅	-0.70%
CheckpointTracker	getProvenCheckpoint	-22 ✅	-0.24%

Full diff report 👇

Contract	Deployment Cost (+/-)	Method	Min (+/-)	%	Avg (+/-)	%	Median (+/-)	%	Max (+/-)	%	# Calls (+/-)
SignalService	2,859,995 (-53,338)	commitmentAt	3,118 (-22)	-0.70%	3,118 (-22)	-0.70%	3,118 (-22)	-0.70%	3,118 (-22)	-0.70%	3 (0)
CheckpointTracker	931,202 (-22)	getProvenCheckpoint proveTransition	9,221 (-22) 96,741 (-44)	-0.24% -0.05%	9,221 (-22) 96,741 (-44)	-0.24% -0.05%	9,221 (-22) 96,741 (-44)	-0.24% -0.05%	9,221 (-22) 96,741 (-44)	-0.24% -0.05%	2 (0) 1 (0)
ProverManager	2,854,874 (0)	prove	103,368 (-22)	-0.02%	125,426 (-22)	-0.02%	127,303 (-22)	-0.02%	139,984 (-22)	-0.02%	6 (0)

src/protocol/ETHBridge.sol

ggonzalez94

Looks great! My only big question is if we should use the same address for the EthBridge on both chains or keep the counterpart variable.
I also complete the TODO by making claimDeposit nonReentrant using transient storage

src/protocol/ETHBridge.sol

Co-authored-by: Gustavo Gonzalez <[email protected]>

…-rollup into decouple-eth-bridge

ggonzalez94

LGTM! I haven't reviewed the tests, but I think we can start merging these PRs and I'll take a look at tests before we merge the big one into main

LeoPatOZ added 13 commits April 30, 2025 11:35

remove authorize in the commitment store

807fa56

height --> index

update to get commitment from a 'trusted' source

717b414

allow genesis to start at the latest publication id

82cf729

dissallow state proofs when verifying signal

aa153b4

start

f025453

add commitment publisher to SS

1b51d2d

remove namespace and decouple bridge

fd2a26c

trusted commitment store

207dd3f

fix test

9e42c20

Merge branch 'simplify-commitment-store' into decouple-eth-bridge

d89d44b

pure

update test

a2b9d2d

Merge branch 'simplify-commitment-store' into decouple-eth-bridge

e5c3a4d

update test

09e908b

LeoPatOZ changed the base branch from main to simplify-commitment-store April 30, 2025 11:35

LeoPatOZ added 2 commits April 30, 2025 15:15

inter

6c2fdd8

update test

6626f47

fix fix test fix test

LeoPatOZ force-pushed the simplify-commitment-store branch from 0cb33a0 to 6626f47 Compare April 30, 2025 15:31

LeoPatOZ added 2 commits April 30, 2025 17:35

Merge branch 'simplify-commitment-store' into decouple-eth-bridge

9eae9dc

update test

290de34

fix fix test fix test single

LeoPatOZ force-pushed the simplify-commitment-store branch from 6626f47 to 290de34 Compare April 30, 2025 15:35

LeoPatOZ and others added 7 commits April 30, 2025 18:32

fix deployment issues in test

44e8e97

fix test 1/n

ac12b20

Merge branch 'simplify-commitment-store' into decouple-eth-bridge

bad54e4

fix bug in libsignal

cccab08

broken

7f14514

Remove unnecessary import

2608edb

nikeshnazareth mentioned this pull request May 5, 2025

[1/2] Simplify commitment store #112

Merged

3 tasks

nikeshnazareth added 2 commits May 6, 2025 12:40

Remove stray "ff" in comment

931c8b0

Fix typo

a22949e

nikeshnazareth and others added 9 commits May 6, 2025 13:44

Add comment explaining address(this) in verifySignal

61cfd98

Fix typo

bc686f2

Remove comment about native bridging from SignalService

f0c4b48

fix comment about state proof in lib signal

d9340f6

improve natspec and remove unnecesary imports

4528794

update comments

9cdac70

update comments

5b58aee

Merge branch 'simplify-commitment-store' into decouple-eth-bridge

35ea231

update comments

7448d95

comment comment better error comment

LeoPatOZ force-pushed the simplify-commitment-store branch from 7645b16 to 7448d95 Compare May 6, 2025 13:46

LeoPatOZ added 4 commits May 6, 2025 15:48

Merge branch 'simplify-commitment-store' into decouple-eth-bridge

252b904

revert changes

0a64605

pure

070991f

Merge branch 'simplify-commitment-store' into decouple-eth-bridge

01b3266

nikeshnazareth added 2 commits May 7, 2025 13:02

Remove obsolete comment

d543b43

Remove obsolete reference to SIGNAL_NAMESPACE

1b77c8f

nikeshnazareth reviewed May 7, 2025

View reviewed changes

src/protocol/ETHBridge.sol Outdated Show resolved Hide resolved

nikeshnazareth and others added 2 commits May 7, 2025 14:55

Bug fix: Use counterpart bridge address to derive signal slot

417f198

add nonReentrant using ReentrancyGuardTransient

0d025ca

ggonzalez94 reviewed May 7, 2025

View reviewed changes

src/protocol/ETHBridge.sol Show resolved Hide resolved

src/protocol/ETHBridge.sol Show resolved Hide resolved

src/protocol/ETHBridge.sol Outdated Show resolved Hide resolved

LeoPatOZ and others added 4 commits May 7, 2025 09:45

better spacing

6a6b292

Update src/protocol/ETHBridge.sol

1c717f9

Co-authored-by: Gustavo Gonzalez <[email protected]>

Merge branch 'decouple-eth-bridge' of github.com:OpenZeppelin/minimal…

dfe6667

…-rollup into decouple-eth-bridge

fix rust

2114335

Base automatically changed from simplify-commitment-store to signal-service May 8, 2025 11:11

Merge branch 'signal-service' into decouple-eth-bridge

f2853c6

ggonzalez94 approved these changes May 8, 2025

View reviewed changes

LeoPatOZ merged commit 56f28ae into signal-service May 8, 2025
4 of 6 checks passed

LeoPatOZ deleted the decouple-eth-bridge branch May 8, 2025 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2/2] Decouple ETH Bridge from signal service #113

[2/2] Decouple ETH Bridge from signal service #113

LeoPatOZ commented Apr 30, 2025 •

edited

Loading

github-actions bot commented May 6, 2025 •

edited

Loading

ggonzalez94 left a comment

ggonzalez94 left a comment

[2/2] Decouple ETH Bridge from signal service #113

[2/2] Decouple ETH Bridge from signal service #113

Conversation

LeoPatOZ commented Apr 30, 2025 • edited Loading

github-actions bot commented May 6, 2025 • edited Loading

Changes to gas cost

🧾 Summary (10% most significant diffs)

ggonzalez94 left a comment

Choose a reason for hiding this comment

ggonzalez94 left a comment

Choose a reason for hiding this comment

LeoPatOZ commented Apr 30, 2025 •

edited

Loading

github-actions bot commented May 6, 2025 •

edited

Loading