Skip to content

Conversation

@anna-carroll
Copy link
Contributor

@anna-carroll anna-carroll commented Jul 14, 2025

Problem

Within Fills, the Output chain ID is intended to indicate the origin chain ID, NOT the destination chain ID.

Confusion arose because the Solidity was designed to "overload" the chainId field on the Output struct depending on the context; in the context of Orders, it should specify the destination chain ID; in the context of Fills, it should specify the origin chain ID.

Previously, the SDK Fill signing logic incorrectly populated Outputs with the destination chain ID.

Fix

Fill Outputs must specify the origin chain ID.

Approach

This PR updates the Fill signing logic in the UnsignedFill struct to explicitly add the origin chain ID before signing.

Alternate Approach

If, at some point in the future, AggregateOrders was used to agg Orders from different origin chains, this logic could become problematic. It may be prudent to address this one step sooner within the AggregateOrders type instead -- by specifying a single origin chain ID for a given AggregateOrders, or else by splitting Orders up within AggregateOrders by respective origin chain IDs. However, this change is more involved.

Open to discussion on which approach is preferred.

@anna-carroll anna-carroll self-assigned this Jul 14, 2025
@anna-carroll anna-carroll requested a review from a team as a code owner July 14, 2025 14:25
@prestwich
Copy link
Member

i don't think the name origin is very descriptive here. i think that the concept we're trying to communicate is what chain the fill is for. i.e. "this can only be used by the chain with id X" the same way that all the other structs/events specify the rollup chain id

@anna-carroll anna-carroll force-pushed the anna/fix-signed-fill branch from 41cd3e0 to ae68156 Compare July 14, 2025 17:28
@prestwich prestwich merged commit 04b4bcd into main Jul 14, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants