Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move DACConfig away from rollup.Config #61

Merged
merged 1 commit into from
Sep 24, 2024
Merged

Move DACConfig away from rollup.Config #61

merged 1 commit into from
Sep 24, 2024

Conversation

blockchaindevsh
Copy link
Collaborator

@blockchaindevsh blockchaindevsh commented Sep 18, 2024

This PR fixes this issue.

UPDATE

rollup.Config is supposed to only contain core configuration values, peripheral parameters such as DACConfig should be passed from CLI instead.

This PR moves DACConfig from rollup.Config to CLI.

if cfg.Driver.SequencerEnabled && cfg.Rollup.IsL2BlobTimeSet() && cfg.DACConfig == nil {
return fmt.Errorf("dac.urls must be set for sequencer when l2 blob time is set")
}
if (!cfg.Driver.SequencerEnabled || !cfg.Rollup.IsL2BlobTimeSet()) && cfg.DACConfig != nil {
Copy link

Choose a reason for hiding this comment

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

Does the full node require DACConfig to download the BLOB?

Copy link
Collaborator Author

@blockchaindevsh blockchaindevsh Sep 19, 2024

Choose a reason for hiding this comment

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

Not needed for full node, the BLOB can be regarded as out of protocol when derivation.

Copy link

Choose a reason for hiding this comment

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

I’m a bit uncertain about this assumption, as it differs from Alt-DA and implies that no roles in the protocol would be responsible for DA challenges. Could you please share the rationale behind this approach?

Copy link
Collaborator Author

@blockchaindevsh blockchaindevsh Sep 19, 2024

Choose a reason for hiding this comment

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

For Alt-DA, they need to get the preimage from L1 commitmemt. But for BLOBs, they're not actually needed by EL in the protocol(when derivation). Interested DAPPs may try to retrieve BLOBs by BLOB hash(from DA server, out of protocol), and do BLOB challenge on L1 if they can't retrieve .

Copy link

Choose a reason for hiding this comment

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

Another way to think about it is by comparing it to the L1 4844 implementation. The L1 EL doesn’t require the BLOB data either, but it is the responsibility of the L1 CL (corresponding to the op-node in the context of L2) to ensure data availability without relying on roles outside the protocol.

Copy link
Collaborator Author

@blockchaindevsh blockchaindevsh Sep 19, 2024

Choose a reason for hiding this comment

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

Yeah the sequencer actually ensures the BLOBs are valid, and submits them to provided DA-server, but not required for normal full nodes.

In summary: BLOBs are in protocol for sequencing, out of protocol for both sequencer and full nodes when derivation.

Copy link

@qizhou qizhou Sep 23, 2024

Choose a reason for hiding this comment

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

Yeah the sequencer actually ensures the BLOBs are valid, and submits them to provided DA-server, but not required for normal full nodes.

In summary: BLOBs are in protocol for sequencing, out of protocol for both sequencer and full nodes when derivation.

I guess out of protocol needs more discussion. The derivation will not check the BLOBs actual data, but will check whether the BLOBs are challenged or not on the alt-DA contract.

In addition, I think op-node should offer a compatible interface as L1 CL, which can retrieve the EIP-4844 BLOBs from the network.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, after BLOB challenge is implemented, BLOBs will be in protocol for both sequencing and deriving.

@GrapeBaBa
Copy link

What's the motivation of this PR, I don't see description in the issue linked.

@blockchaindevsh
Copy link
Collaborator Author

What's the motivation of this PR, I don't see description in the issue linked.

It's a refactor of existing code, DACConfig should not be included in rollup config.

@@ -402,7 +402,8 @@ func (n *OpNode) initL2(ctx context.Context, cfg *Config, snapshotLog log.Logger
} else {
n.safeDB = safedb.Disabled
}
n.l2Driver = driver.NewDriver(&cfg.Driver, &cfg.Rollup, n.l2Source, n.l1Source, n.beacon, n, n, n.log, snapshotLog, n.metrics, cfg.ConfigPersistence, n.safeDB, &cfg.Sync, sequencerConductor, plasmaDA)
dacClient := cfg.DACConfig.Client()

Choose a reason for hiding this comment

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

It seems most of the components such as l1source, l2source managed by OpNode, what about making dacClient also a field in OpNode?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think it's necessary, it's handled similar to plasmaDA in code.

Choose a reason for hiding this comment

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

Ok.

URLS []string
}

func (dacConfig *DACConfig) Client() derive.DACClient {

Choose a reason for hiding this comment

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

It is unusual using config object as a receiver for creating a client.

@@ -1192,7 +1192,7 @@ func TestEngineQueue_StepPopOlderUnsafe(t *testing.T) {

prev := &fakeAttributesQueue{origin: refA}

ec := NewEngineController(eng, logger, metrics.NoopMetrics, &rollup.Config{}, sync.CLSync)
ec := NewEngineController(eng, logger, metrics.NoopMetrics, &rollup.Config{}, sync.CLSync, nil)

Choose a reason for hiding this comment

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

All the tests use the nil daclient arg, it seems not easy to test this feature

Copy link
Collaborator Author

@blockchaindevsh blockchaindevsh Sep 19, 2024

Choose a reason for hiding this comment

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

I think we didn't write test for every trivial change or changes with high confidence because of limited engineering resources before(and to keep the diff with upstream small, but did test manually), but as more team members join, we can add more test in the future.

Choose a reason for hiding this comment

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

manual test means test the functionality on the testnet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes.

@@ -44,6 +44,10 @@ type ExecEngine interface {
L2BlockRefByLabel(ctx context.Context, label eth.BlockLabel) (eth.L2BlockRef, error)
}

type DACClient interface {

Choose a reason for hiding this comment

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

Do we have a retry and timeout config to make the remote call robust?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ctx parameter can be used for timeout, if this call fails, it'll retry .

Choose a reason for hiding this comment

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

Got it.

@GrapeBaBa
Copy link

Does it mean force transaction can't use this feature?

@blockchaindevsh
Copy link
Collaborator Author

Does it mean force transaction can't use this feature?

What do you mean by force transaction?

@GrapeBaBa
Copy link

Does it mean force transaction can't use this feature?

What do you mean by force transaction?

Transaction from L1, not through sequencer.

@blockchaindevsh
Copy link
Collaborator Author

Does it mean force transaction can't use this feature?

What do you mean by force transaction?

Transaction from L1, not through sequencer.

This option is only for L2 BLOBs.

@blockchaindevsh
Copy link
Collaborator Author

@qzhodl @qizhou Do you have any further advice for this PR? I'd like to get it merged before starting to merge with upstream monorepo.

@qizhou
Copy link

qizhou commented Sep 23, 2024

I would suggest adding more descriptions of this PR, such as why the config is moved out of the rollup config. What is the current solution.

@blockchaindevsh
Copy link
Collaborator Author

I would suggest adding more descriptions of this PR, such as why the config is moved out of the rollup config. What is the current solution.

Done.

@qizhou
Copy link

qizhou commented Sep 23, 2024

I would suggest adding more descriptions of this PR, such as why the config is moved out of the rollup config. What is the current solution.

Done.

Looks much better.

One thing I would like to mention is that DAC list is a runtime config, like bootnodes in op-geth. In fact, we should set some default DAC list given the chain ID similar to bootnodes:

@blockchaindevsh
Copy link
Collaborator Author

blockchaindevsh commented Sep 23, 2024

In fact, we should set some default DAC list given the chain ID similar to bootnodes:

Plasma DA urls is purely read from CLI(source), so maybe it's better to stick to that ?

@qizhou
Copy link

qizhou commented Sep 23, 2024

In fact, we should set some default DAC list given the chain ID similar to bootnodes:

Plasma DA urls is purely read from CLI(source), so maybe it's better to stick to that ?

I guess the reason is that, unlike bootnodes, which is a must in a EL node, Plasma DA is not yet popular these days

@blockchaindevsh blockchaindevsh merged commit 7982f02 into op-es Sep 24, 2024
2 checks passed
@blockchaindevsh blockchaindevsh deleted the fix_dac branch September 24, 2024 00:53
qizhou pushed a commit that referenced this pull request Oct 17, 2024
…-optimism#12321)

* feat: add superchain erc20 bridge (#61)

* feat: add superchain erc20 bridge

* fix: interfaces and versions

* refactor: optimism superchain erc20 redesign (#62)

* refactor: use oz upgradeable erc20 as dependency

* chore: update interfaces

* fix: tests based on changes

* refactor: remove op as dependency

* feat: add check for supererc20 bridge on modifier

* chore: update tests and interfaces

* chore: update stack vars name on test

* chore: remove empty gitmodules file

* chore: update superchain weth errors

* test: add superchain erc20 bridge tests (#65)

* test: add superchain erc20 bridge tests

* test: add optimism superchain erc20 beacon tests

* test: remove unnecessary test

* test: tests fixes

* test: tests fixes

* chore: update missing bridge on natspec (#69)

* chore: update missing bridge on natspec

* fix: natspecs

---------

Co-authored-by: agusduha <[email protected]>

* fix: remove superchain erc20 base (#70)

* refactor: update isuperchainweth (#71)


---------

Co-authored-by: agusduha <[email protected]>

* feat: rename mint/burn and add SuperchainERC20 (#74)

* refactor: rename mint and burn functions on superchain erc20

* chore: rename optimism superchain erc20 to superchain erc20

* feat: create optimism superchain erc20 contract

* chore: update natspec and errors

* fix: superchain erc20 tests

* refactor: make superchain erc20 abstract

* refactor: move storage and erc20 metadata functions to implementation

* chore: update interfaces

* chore: update superchain erc20 events

* fix: tests

* fix: natspecs

* fix: add semmver lock and snapshots

* fix: remove unused imports

* fix: natspecs

---------

Co-authored-by: 0xDiscotech <[email protected]>

* fix: refactor zero check (#76)

* fix: pre pr

* fix: semver natspec check failure (#79)

* fix: semver natspec check failure

* fix: ignore mock contracts in semver natspec script

* fix: error message

* feat: add crosschain erc20 interface (#80)

* feat: add crosschain erc20 interface

* fix: refactor interfaces

* fix: superchain bridge natspec (#83)

* fix: superchain weth natspec (#84)

Co-authored-by: 0xng <[email protected]>
Co-authored-by: 0xParticle <[email protected]>
Co-authored-by: gotzenx <[email protected]>

* fix: stop inheriting superchain interfaces (#85)

* fix: stop inheriting superchain interfaces

* fix: move events and erros into the implementation

* fix: make superchainERC20 inherits from crosschainERC20

* fix: superchain bridge rename (#86)

* fix: fee vault compiler error (#87)

* fix: remove unused imports

* fix: refactor common errors (#90)

* fix: refactor common errors

* fix: remove unused version

* fix: reuse unauthorized error (#92)

* fix: superchain erc20 factory conflicts

* fix: rename crosschain functions (#94)

---------

Co-authored-by: Disco <[email protected]>
Co-authored-by: 0xng <[email protected]>
Co-authored-by: 0xParticle <[email protected]>
Co-authored-by: gotzenx <[email protected]>
qizhou pushed a commit that referenced this pull request Oct 23, 2024
)

* feat: add superchain erc20 bridge (#61)

* feat: add superchain erc20 bridge

* fix: interfaces and versions

* refactor: optimism superchain erc20 redesign (#62)

* refactor: use oz upgradeable erc20 as dependency

* chore: update interfaces

* fix: tests based on changes

* refactor: remove op as dependency

* feat: add check for supererc20 bridge on modifier

* chore: update tests and interfaces

* chore: update stack vars name on test

* chore: remove empty gitmodules file

* chore: update superchain weth errors

* test: add superchain erc20 bridge tests (#65)

* test: add superchain erc20 bridge tests

* test: add optimism superchain erc20 beacon tests

* test: remove unnecessary test

* test: tests fixes

* test: tests fixes

* chore: update missing bridge on natspec (#69)

* chore: update missing bridge on natspec

* fix: natspecs

---------

Co-authored-by: agusduha <[email protected]>

* fix: remove superchain erc20 base (#70)

* refactor: update isuperchainweth (#71)


---------

Co-authored-by: agusduha <[email protected]>

* feat: rename mint/burn and add SuperchainERC20 (#74)

* refactor: rename mint and burn functions on superchain erc20

* chore: rename optimism superchain erc20 to superchain erc20

* feat: create optimism superchain erc20 contract

* chore: update natspec and errors

* fix: superchain erc20 tests

* refactor: make superchain erc20 abstract

* refactor: move storage and erc20 metadata functions to implementation

* chore: update interfaces

* chore: update superchain erc20 events

* fix: tests

* fix: natspecs

* fix: add semmver lock and snapshots

* fix: remove unused imports

* fix: natspecs

---------

Co-authored-by: 0xDiscotech <[email protected]>

* fix: refactor zero check (#76)

* fix: pre pr

* fix: semver natspec check failure (#79)

* fix: semver natspec check failure

* fix: ignore mock contracts in semver natspec script

* fix: error message

* feat: add crosschain erc20 interface (#80)

* feat: add crosschain erc20 interface

* fix: refactor interfaces

* fix: superchain bridge natspec (#83)

* fix: superchain weth natspec (#84)

Co-authored-by: 0xng <[email protected]>
Co-authored-by: 0xParticle <[email protected]>
Co-authored-by: gotzenx <[email protected]>

* fix: stop inheriting superchain interfaces (#85)

* fix: stop inheriting superchain interfaces

* fix: move events and erros into the implementation

* fix: make superchainERC20 inherits from crosschainERC20

* fix: superchain bridge rename (#86)

* fix: fee vault compiler error (#87)

* fix: remove unused imports

* fix: refactor common errors (#90)

* fix: refactor common errors

* fix: remove unused version

* feat: add cross domain context function

* fix: reuse unauthorized error (#92)

* fix: superchain erc20 factory conflicts

* fix: rename crosschain functions (#94)

* chore: run pre-pr

* chore: run pre-pr

* fix: mocked calls on tests

* feat: add cross domain message context function (#98)


----
Co-Authored-by: AgusDuha <[email protected]>

---------

Co-authored-by: AgusDuha <[email protected]>
Co-authored-by: agusduha <[email protected]>
Co-authored-by: 0xng <[email protected]>
Co-authored-by: 0xParticle <[email protected]>
Co-authored-by: gotzenx <[email protected]>
qizhou pushed a commit that referenced this pull request Oct 23, 2024
* feat: SuperchainWETH redesign (#101)

* feat: add superchain erc20 bridge (#61)

* feat: add superchain erc20 bridge

* fix: interfaces and versions

* refactor: optimism superchain erc20 redesign (#62)

* refactor: use oz upgradeable erc20 as dependency

* chore: update interfaces

* fix: tests based on changes

* refactor: remove op as dependency

* feat: add check for supererc20 bridge on modifier

* chore: update tests and interfaces

* chore: update stack vars name on test

* chore: remove empty gitmodules file

* chore: update superchain weth errors

* test: add superchain erc20 bridge tests (#65)

* test: add superchain erc20 bridge tests

* test: add optimism superchain erc20 beacon tests

* test: remove unnecessary test

* test: tests fixes

* test: tests fixes

* chore: update missing bridge on natspec (#69)

* chore: update missing bridge on natspec

* fix: natspecs

---------

Co-authored-by: agusduha <[email protected]>

* fix: remove superchain erc20 base (#70)

* refactor: update isuperchainweth (#71)


---------

Co-authored-by: agusduha <[email protected]>

* feat: rename mint/burn and add SuperchainERC20 (#74)

* refactor: rename mint and burn functions on superchain erc20

* chore: rename optimism superchain erc20 to superchain erc20

* feat: create optimism superchain erc20 contract

* chore: update natspec and errors

* fix: superchain erc20 tests

* refactor: make superchain erc20 abstract

* refactor: move storage and erc20 metadata functions to implementation

* chore: update interfaces

* chore: update superchain erc20 events

* fix: tests

* fix: natspecs

* fix: add semmver lock and snapshots

* fix: remove unused imports

* fix: natspecs

---------

Co-authored-by: 0xDiscotech <[email protected]>

* fix: refactor zero check (#76)

* fix: pre pr

* fix: semver natspec check failure (#79)

* fix: semver natspec check failure

* fix: ignore mock contracts in semver natspec script

* fix: error message

* feat: add crosschain erc20 interface (#80)

* feat: add crosschain erc20 interface

* fix: refactor interfaces

* fix: superchain bridge natspec (#83)

* fix: superchain weth natspec (#84)

Co-authored-by: 0xng <[email protected]>
Co-authored-by: 0xParticle <[email protected]>
Co-authored-by: gotzenx <[email protected]>

* fix: stop inheriting superchain interfaces (#85)

* fix: stop inheriting superchain interfaces

* fix: move events and erros into the implementation

* fix: make superchainERC20 inherits from crosschainERC20

* fix: superchain bridge rename (#86)

* fix: fee vault compiler error (#87)

* fix: remove unused imports

* fix: refactor common errors (#90)

* fix: refactor common errors

* fix: remove unused version

* fix: reuse unauthorized error (#92)

* fix: superchain erc20 factory conflicts

* fix: rename crosschain functions (#94)

* feat: superweth redesign

* fix: pr fixes

* fix: fixes post merge

---------

Co-authored-by: Disco <[email protected]>
Co-authored-by: 0xng <[email protected]>
Co-authored-by: 0xParticle <[email protected]>
Co-authored-by: gotzenx <[email protected]>

* fix: SuperchainWETH redesign fixes (#110)

* fix: superchainWETH redesign fixes

* fix: withdraw arg

* fix: fix revert in SuperchainWETH tests (#112)

---------

Co-authored-by: AgusDuha <[email protected]>
Co-authored-by: Disco <[email protected]>
Co-authored-by: 0xng <[email protected]>
Co-authored-by: 0xParticle <[email protected]>
Co-authored-by: agusduha <[email protected]>
qizhou pushed a commit that referenced this pull request Oct 23, 2024
…rc20 implementation (ethereum-optimism#12476)

* feat: add superchain erc20 bridge (#61)

* feat: add superchain erc20 bridge

* fix: interfaces and versions

* refactor: optimism superchain erc20 redesign (#62)

* refactor: use oz upgradeable erc20 as dependency

* chore: update interfaces

* fix: tests based on changes

* refactor: remove op as dependency

* feat: add check for supererc20 bridge on modifier

* chore: update tests and interfaces

* chore: update stack vars name on test

* chore: remove empty gitmodules file

* chore: update superchain weth errors

* test: add superchain erc20 bridge tests (#65)

* test: add superchain erc20 bridge tests

* test: add optimism superchain erc20 beacon tests

* test: remove unnecessary test

* test: tests fixes

* test: tests fixes

* chore: update missing bridge on natspec (#69)

* chore: update missing bridge on natspec

* fix: natspecs

---------

Co-authored-by: agusduha <[email protected]>

* fix: remove superchain erc20 base (#70)

* refactor: update isuperchainweth (#71)


---------

Co-authored-by: agusduha <[email protected]>

* feat: rename mint/burn and add SuperchainERC20 (#74)

* refactor: rename mint and burn functions on superchain erc20

* chore: rename optimism superchain erc20 to superchain erc20

* feat: create optimism superchain erc20 contract

* chore: update natspec and errors

* fix: superchain erc20 tests

* refactor: make superchain erc20 abstract

* refactor: move storage and erc20 metadata functions to implementation

* chore: update interfaces

* chore: update superchain erc20 events

* fix: tests

* fix: natspecs

* fix: add semmver lock and snapshots

* fix: remove unused imports

* fix: natspecs

---------

Co-authored-by: 0xDiscotech <[email protected]>

* fix: refactor zero check (#76)

* fix: pre pr

* chore: add new solady version and import it for erc20

* fix: undo forge std changes

* chore: re run pre pr script

* fix: semver natspec check failure (#79)

* fix: semver natspec check failure

* fix: ignore mock contracts in semver natspec script

* fix: error message

* feat: add crosschain erc20 interface (#80)

* feat: add crosschain erc20 interface

* fix: refactor interfaces

* fix: superchain bridge natspec (#83)

* fix: superchain weth natspec (#84)

Co-authored-by: 0xng <[email protected]>
Co-authored-by: 0xParticle <[email protected]>
Co-authored-by: gotzenx <[email protected]>

* fix: stop inheriting superchain interfaces (#85)

* fix: stop inheriting superchain interfaces

* fix: move events and erros into the implementation

* fix: make superchainERC20 inherits from crosschainERC20

* fix: superchain bridge rename (#86)

* fix: fee vault compiler error (#87)

* fix: remove unused imports

* chore: run pre-pr and update vendor interface

* fix: refactor common errors (#90)

* fix: refactor common errors

* fix: remove unused version

* feat: add permit2 on optimism superchain erc20

* chore: run pre-pr script

* fix: reuse unauthorized error (#92)

* fix: superchain erc20 factory conflicts

* fix: rename crosschain functions (#94)

* chore: run pre-pr

* chore: run pre-pr

* chore: run pre-pr

* feat: add new tests on optimism superchain erc20

* fix: vars and params naming on newly added tests

* fix: var name

* feat: support permit2 on optimism superchain erc20 and upgrade solady's erc20 implementation (#97)


---
Co-Authored-by: AgusDuha <[email protected]>

* chore: use ierc20 alias for ierc20 solady interface (#108)

---------

Co-authored-by: AgusDuha <[email protected]>
Co-authored-by: agusduha <[email protected]>
Co-authored-by: 0xng <[email protected]>
Co-authored-by: 0xParticle <[email protected]>
Co-authored-by: gotzenx <[email protected]>
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.

5 participants