Skip to content

Conversation

@daniilrrr
Copy link
Collaborator

Ticket

  • Related Linear Ticket: SEQ-117

What does this PR do?

  • Summary: Small refactor to generate Rust bindings from the source contract now that we have a monorepo

Does this PR introduce any breaking changes (API/schema)?

  • No

Do any environment variables need to be updated or added before deployment?

  • No

How can this PR be tested?

  • Nothing should change

@linear
Copy link

linear bot commented Oct 25, 2024

SEQ-117 Handle transactions that must be split up into parts

The max size of a Solidity transaction is 128KB, but we're limited to ~90KB of data due to the overhead in formatting a bulk smart contract call. We need to split transactions that are too large up into parts.

See https://ethereum.stackexchange.com/a/144128 for the 128KB limit in Solidity and SEQ-103 for the 90KB limit in our smart contracts.

This should be done in a way that if the max gas changes (because e.g. more modules are added, lowering the room we have for events), the system seamlessly adapts and handles the lower max limit. We can't assume that the max limit will be a particular KB threshold, since we can't know what modules are in use (and therefore how much gas/throughput we'll have available).

Note that we will also be compressing transaction data using brotli compression, and compressing prior to chunking will likely be more fruitful than chunking then compression. The backfill project is using this for their TX lists, and we can reuse the same logic.

Acceptance criteria:

  • Our sequencer can properly handle transactions that are 128KB in size
  • Our sequencer can break transactions into parts regardless of the size of the module
  • Make sure sequencer's process_bulk_transactions() interface matches the updated contract (SEQ-105) and handles "this txn is M of N chunks"

@RomanHodulak
Copy link
Contributor

Oh cool!

One thing to note here is that making changes to Solidity contracts now breaks the Rust codebase, which may impose some coordination overhead - changing the smart contract interface breaks the build and prevents merging the changes until the Rust codebase is also updated.

As a stylistic nit, I'd prefer to keep the implementation out of the mod.rs and put it into a designated module like sol.rs is for solidity implementation.

@daniilrrr
Copy link
Collaborator Author

daniilrrr commented Oct 28, 2024

Oh cool!

One thing to note here is that making changes to Solidity contracts now breaks the Rust codebase, which may impose some coordination overhead - changing the smart contract interface breaks the build and prevents merging the changes until the Rust codebase is also updated.

Good callout @RomanHodulak . That's a bummer and imo we shouldn't impose this coupling on the rest of the team. I do think there is value in generating the Rust bindings directly from the source contract.

Potential solution

  1. Maintain a copy of the full .sol file within the metabased-rollup/metabased-sequencer subfolder
  2. Have some kind of mechanism (not quite a unit test since that would block PRs, so maybe just a printing function?) that checks for equality between the metabased-contracts and metabased-sequencer version, and complains if there's a mismatch.

Can you think of any other ways to handle this?

EDIT:
We could also put it in the /shared folder part of SEQ-250

@daniilrrr daniilrrr force-pushed the daniil/SEQ-117-n01-derive-from-contract- branch from 92f0d08 to cc2a958 Compare October 28, 2024 18:20
Copy link
Contributor

@WillPapper WillPapper left a comment

Choose a reason for hiding this comment

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

Cool macro! Looks much simpler now

Comment on lines +7 to +12
use alloy::sol;
sol!(
#[sol(rpc)]
"../../metabased-contracts/src/MetabasedSequencerChain.sol"
);
pub use MetabasedSequencerChain as MetabasedSequencerChainInstance;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really cool! I like how it handles automatic ABI generation

We should document this functionality by linking to https://alloy.rs/highlights/the-sol!-procedural-macro.html, since people new to the codebase are likely unfamiliar with the sol! macro

@WillPapper
Copy link
Contributor

WillPapper commented Oct 28, 2024

  1. Maintain a copy of the full .sol file within the metabased-rollup/metabased-sequencer subfolder
  2. Have some kind of mechanism (not quite a unit test since that would block PRs, so maybe just a printing function?) that checks for equality between the metabased-contracts and metabased-sequencer version, and complains if there's a mismatch.

Can you think of any other ways to handle this?

EDIT: We could also put it in the /shared folder part of SEQ-250

There is another option, which is to always version our contracts on the Solidity side. We've done this for a while, since you often need older versions of contracts for verification purposes. Once a contract is "released" we can move it into a subdirectory where it remains versioned and is always unmodified.

In that case, /metabased-rollup/metabased-contracts/src/MetabasedSequencerChain.sol contains the current version, and /metabased-rollup/metabased-contracts/src/releases/MetabasedSequencerChainV1-2.sol (or similar) contains the older "released" versions. If we only reference the released versions throughout the codebase, we won't have to worry about breaking changes.

@WillPapper
Copy link
Contributor

@gustavoguimaraes Do you have thoughts on the versioning questions above?

@gustavoguimaraes
Copy link
Contributor

@gustavoguimaraes Do you have thoughts on the versioning questions above?

I like the release version idea you proposed @WillPapper . I can start doing it one this PR is merged.

@RomanHodulak
Copy link
Contributor

The metabased-contracts is the upstream and the metabased-sequencer is the downstream for sequencer chain solidity contract. We want to decouple the versioning of upstream and downstream. This can be achieved by:

Generating the rust bindings using:

  1. The contract header source in the sol! macro on the sequencer side (the current solution)
  2. An immutable published ABI (Will's proposal)

The main issue with 1. is duplication of knowledge - since we're keeping the contract interface on two places that need to be in sync. Although nice thing is that we can declare only what we need and since we're making it by hand it is forcing us to think about the changes being made and what needs to be updated on call sites.

This issue is solved in 2. as we get one source of truth / knowledge. We can observe changes and go through them by comparing the release source files. Although we need to filter out changes not relevant to the downstream code.

| I like the release version idea you proposed @WillPapper . I can start doing it one this PR is merged.

@gustavoguimaraes I think it would be better to first make the immutable releases and then followup with PR on the rust side that consumes it. I don't think we need to merge this PR in this state nor that it blocks this effort. Wdyt?

@gustavoguimaraes
Copy link
Contributor

The metabased-contracts is the upstream and the metabased-sequencer is the downstream for sequencer chain solidity contract. We want to decouple the versioning of upstream and downstream. This can be achieved by:

Generating the rust bindings using:

  1. The contract header source in the sol! macro on the sequencer side (the current solution)
  2. An immutable published ABI (Will's proposal)

The main issue with 1. is duplication of knowledge - since we're keeping the contract interface on two places that need to be in sync. Although nice thing is that we can declare only what we need and since we're making it by hand it is forcing us to think about the changes being made and what needs to be updated on call sites.

This issue is solved in 2. as we get one source of truth / knowledge. We can observe changes and go through them by comparing the release source files. Although we need to filter out changes not relevant to the downstream code.

| I like the release version idea you proposed @WillPapper . I can start doing it one this PR is merged.

@gustavoguimaraes I think it would be better to first make the immutable releases and then followup with PR on the rust side that consumes it. I don't think we need to merge this PR in this state nor that it blocks this effort. Wdyt?

Sounds good @RomanHodulak 👍

@daniilrrr daniilrrr requested a review from WillPapper October 30, 2024 19:35
@daniilrrr
Copy link
Collaborator Author

daniilrrr commented Oct 30, 2024

HOLD on this until contract updates are immutable+versioned

@daniilrrr
Copy link
Collaborator Author

  1. Maintain a copy of the full .sol file within the metabased-rollup/metabased-sequencer subfolder
  2. Have some kind of mechanism (not quite a unit test since that would block PRs, so maybe just a printing function?) that checks for equality between the metabased-contracts and metabased-sequencer version, and complains if there's a mismatch.

Can you think of any other ways to handle this?
EDIT: We could also put it in the /shared folder part of SEQ-250

There is another option, which is to always version our contracts on the Solidity side. We've done this for a while, since you often need older versions of contracts for verification purposes. Once a contract is "released" we can move it into a subdirectory where it remains versioned and is always unmodified.

In that case, /metabased-rollup/metabased-contracts/src/MetabasedSequencerChain.sol contains the current version, and /metabased-rollup/metabased-contracts/src/releases/MetabasedSequencerChainV1-2.sol (or similar) contains the older "released" versions. If we only reference the released versions throughout the codebase, we won't have to worry about breaking changes.

chatted with @gustavoguimaraes and he'll work on a PR to solve for this while also avoiding repo bloat from old contract versions

@daniilrrr
Copy link
Collaborator Author

daniilrrr commented Jan 17, 2025

@WillPapper suggested using a specific commit hash of the contracts here so let's try and use that

@jorgemmsilva
Copy link
Collaborator

we're currently generating bindings for these contracts here:
https://github.com/SyndicateProtocol/metabased-rollup/blob/main/metabased-translator/Makefile#L11-L15

please take it into account. We might want to move them to a more common place at the project root that both translator and sequencer can access.

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.

7 participants