Skip to content

feat: example code to sign & send Orders - #44

Merged
anna-carroll merged 2 commits into
mainfrom
anna/order-ex
May 12, 2025
Merged

anna-carroll merged 2 commits into
mainfrom
anna/order-ex

Conversation

@anna-carroll

@anna-carroll anna-carroll commented May 1, 2025 •

Copy link
Copy Markdown
Contributor
  • construct a simple example Order
  • sign the Order
  • send the Order
  • drive-by: update Filler code to use SignetSystemConstants

@anna-carroll
anna-carroll requested a review from a team as a code owner May 1, 2025 10:09
@anna-carroll anna-carroll changed the title feat feat: example code to sign & send Orders May 1, 2025
@anna-carroll anna-carroll self-assigned this May 1, 2025
Comment thread crates/rpc/examples/order.rs Resolved
Comment thread crates/rpc/examples/order.rs Resolved
Comment thread crates/rpc/examples/order.rs Resolved
Comment thread crates/rpc/examples/order.rs Resolved
Comment thread crates/rpc/examples/order.rs Resolved
Base automatically changed from anna/filler-ex to main May 7, 2025 14:18
}

/// Get the Order contract address for the given chain id.
pub const fn orders_for(&self, chain_id: u64) -> Option<Address> {

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.

not sure if this is the most idiomatic way to do this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the fn signature is idiomatic, but it's kinda weird in that there are exactly 2 choices, and the caller will always know which they want, so why do we allow passing an arbitrary chain id? caller should probably use .host_orders() or .rollup_orders() directly instead?

@prestwich prestwich May 9, 2025 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it's kinda fine this way I guess? the agg may contain invalid or junk data and this is one of the better ways to handle that I suppose

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yeah i wish we didn't have an arbitrary chain id here, but james's argument is sound

@Evalir Evalir left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

basically lgtm, just the convo about the chain id

}

/// Get the Order contract address for the given chain id.
pub const fn orders_for(&self, chain_id: u64) -> Option<Address> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yeah i wish we didn't have an arbitrary chain id here, but james's argument is sound

@anna-carroll
anna-carroll merged commit 59dffe1 into main May 12, 2025
@anna-carroll
anna-carroll deleted the anna/order-ex branch May 12, 2025 12:02
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