Skip to content

feat!: make proposals EIP-712#22531

Merged
spalladino merged 3 commits intomerge-train/spartanfrom
lh/tmnt-520-make-it-stupid-simple-to-check-cross-chain-replay-for
Apr 28, 2026
Merged

feat!: make proposals EIP-712#22531
spalladino merged 3 commits intomerge-train/spartanfrom
lh/tmnt-520-make-it-stupid-simple-to-check-cross-chain-replay-for

Conversation

@LHerskind
Copy link
Copy Markdown
Contributor

This changes proposal-path validator signatures from eth_sign / personal_sign style payload signing to EIP-712 typed-data signing. Proposal and attestation signatures are now bound to both the L1 chainId and the rollup contract address, which closes cross-chain and cross-rollup replay for block proposals, checkpoint proposals, checkpoint attestations, and the proposer signature over packed attestations/signers.

On the Solidity side, the rollup path now computes EIP-712 digests through CoordinationSignatureLib.sol, reusing the existing Aztec Rollup domain name and version 1, and threads the verifying contract explicitly where needed. On the TypeScript side, proposal-path signing and recovery now require a { chainId, rollupAddress } signature context, validator duties sign typed data instead of raw messages, and the shared helper surface was renamed to CoordinationSignature* to match Solidity.

Tests were updated to check digest parity between TS and Solidity, sender recovery under the correct domain, failure under the wrong chainId or rollup address, and the affected contract flows that verify or invalidate checkpoint-related signatures.

Historically, it was possible to reason about replay from archives and chain history, but it was not always stupid-simple. With the EIP-712 domain binding, the replay boundary is explicit in the signature itself, so checking whether a signature targets the wrong chain or wrong rollup becomes straightforward and only depends on current L1 state.

@LHerskind LHerskind added the ci-draft Run CI on draft PRs. label Apr 14, 2026
@LHerskind LHerskind force-pushed the lh/tmnt-520-make-it-stupid-simple-to-check-cross-chain-replay-for branch 5 times, most recently from a8ec1a0 to fb34548 Compare April 15, 2026 07:11
@LHerskind LHerskind marked this pull request as ready for review April 15, 2026 08:58
@LHerskind LHerskind force-pushed the lh/tmnt-520-make-it-stupid-simple-to-check-cross-chain-replay-for branch 2 times, most recently from 5a02d69 to cfe2a60 Compare April 15, 2026 12:50
Copy link
Copy Markdown
Contributor

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

Looks solid. Just two things I'd want to discuss:

1- Should we drop the SignatureDomainSeparator given EIP712 includes the typehash? Feels redundant.

2- Should we explicitly include the chainid and rollup version in the proposal and attestation p2p structs so we can easily validate them, and at the same time we don't need a coordination context on any caller of getSender? If not, then at least I'd shoot for having two different structs: the current one that gets moved throughout p2p, and a "resolved" one where the sender is the computed value, which we use internally.

ProposePayload({archive: proposeArgs.archive, oracleInput: proposeArgs.oracleInput, headerHash: headerHash});

bytes32 digest = ProposeLib.digest(proposePayload);
bytes32 digest = ProposeLib.digest(proposePayload, address(rollup));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was about to ask whether you needed both version of the digest method, with and without the verifying contract address.

Comment on lines +21 to +23
function domainSeparator(address _verifyingContract) internal view returns (bytes32) {
return keccak256(abi.encode(DOMAIN_TYPEHASH, NAME_HASH, VERSION_HASH, block.chainid, _verifyingContract));
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we cache this as an immutable?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes-but-no

We can, but since we are using it from libraries and not where the storage are, it is a bit of a pain as the library don't have a constructor where we could do it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm pleasantly surprised the increase is so small, nice!

address _verifyingContract
) internal view returns (bytes32) {
return CoordinationSignatureLib.attestationsAndSignersDigest(
keccak256(abi.encode(SignatureDomainSeparator.attestationsAndSigners, _attestations, _signers)),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we still need the SignatureDomainSeparator if we have the 712 typehash?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Atm its mainly dispatching and then because of the SignedTxs that it was leftover, but will look at also making those EIP-712 so it is just one path.

@LHerskind
Copy link
Copy Markdown
Contributor Author

  1. Should we drop the SignatureDomainSeparator given EIP712 includes the typehash? Feels redundant.

We did not use EIP712 all over, so am looking to use it more for the SignedTxs.

2 Should we explicitly include the chainid and rollup version in the proposal and attestation p2p structs so we can easily validate them, and at the same time we don't need a coordination context on any caller of getSender? If not, then at least I'd shoot for having two different structs: the current one that gets moved throughout p2p, and a "resolved" one where the sender is the computed value, which we use internally.

I'm taking a look at it now should be able to simplify nicely together with the fix in 1.

@LHerskind LHerskind force-pushed the lh/tmnt-520-make-it-stupid-simple-to-check-cross-chain-replay-for branch 3 times, most recently from 7003b54 to 7256238 Compare April 16, 2026 18:21
Copy link
Copy Markdown
Contributor

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few nits. And sorry I took so long to re-review.

expectedChainId: this.signatureContext.chainId,
expectedRollupAddress: this.signatureContext.rollupAddress.toString(),
});
return { result: 'reject', severity: PeerErrorSeverity.HighToleranceError };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
return { result: 'reject', severity: PeerErrorSeverity.HighToleranceError };
return { result: 'reject', severity: PeerErrorSeverity.LowToleranceError };

expectedChainId: this.signatureContext.chainId,
expectedRollupAddress: this.signatureContext.rollupAddress.toString(),
});
return { result: 'reject', severity: PeerErrorSeverity.HighToleranceError };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same


constructor(
public attestations: CommitteeAttestation[],
public readonly signatureContext: CoordinationSignatureContext = EMPTY_COORDINATION_SIGNATURE_CONTEXT,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it break too many things if we remove the default? I'm worried we forget to wire it in in a legitimate code path.

| undefined;
readonly primaryType: CoordinationSignatureType = 'BlockProposal';

private cachedSender: EthAddress | undefined | null = null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: I'd default to undefined for initial value, and null if no value is recoverable, or even a null-object

@spalladino
Copy link
Copy Markdown
Contributor

By the way, do you want to move this to merge-train/spartan?

@LHerskind LHerskind force-pushed the lh/tmnt-520-make-it-stupid-simple-to-check-cross-chain-replay-for branch from 7256238 to 41f782d Compare April 28, 2026 11:55
@LHerskind LHerskind changed the base branch from next to merge-train/spartan April 28, 2026 11:55
@LHerskind LHerskind force-pushed the lh/tmnt-520-make-it-stupid-simple-to-check-cross-chain-replay-for branch from 41f782d to 63e0b5c Compare April 28, 2026 11:59
@spalladino spalladino merged commit 625f9f2 into merge-train/spartan Apr 28, 2026
12 checks passed
@spalladino spalladino deleted the lh/tmnt-520-make-it-stupid-simple-to-check-cross-chain-replay-for branch April 28, 2026 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-draft Run CI on draft PRs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants