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.

Sign up for GitHub

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

Jump to bottom

draft: ban-deposits-interop first RFC draft #11362

Closed
wants to merge 34 commits into from
Closed

draft: ban-deposits-interop first RFC draft #11362

wants to merge 34 commits into from

Conversation

skeletor-spaceman
Copy link
Collaborator

@skeletor-spaceman skeletor-spaceman commented Aug 5, 2024 •
edited
Loading

Description

This draft PR should NOT be merged, it is here to serve as a place to leave feedback and collaborate on implementation ideas.

To get ready for Interop, we need to make sure to disable ExecutingMessages to be referenced by Deposit transactions.

This PR touches both the client and the relevant pre-deploy contracts.

more context: ticket & spec

Tests

Tests will NOT be added to this PR, as it only serves as a collaborative draft.

Additional context

Missing features:

  • !ba.rollupCfg.IsInteropActivationBlock legacy deposit handling
  • some marshall and unmarshall functionality for DepositsCompleteTx
  • Interop l1InfoTx
  • add natspec
  • Modify SetL1BlockValues function on .go files
  • client tests feat: ban deposits client tests #11504

Metadata

@skeletor-spaceman skeletor-spaceman requested a review from tynes August 5, 2024 23:17
@tynes
Copy link
Contributor

tynes commented Aug 5, 2024

There is currently some discrepancies between using IsInterop vs IsIsthmus - we need to standardize the usage, just to be mindful of

@skeletor-spaceman
Copy link
Collaborator Author

skeletor-spaceman commented Aug 6, 2024 •
edited
Loading

There is currently some discrepancies between using IsInterop vs IsIsthmus - we need to standardize the usage, just to be mindful of

should we also modify:

optimism/op-node/rollup/types.go

Line 492 in 0d78e5a

func (c *Config) IsInteropActivationBlock(l2BlockTime uint64) bool {
?

maybe on another PR.

for now I'll use IsInteropActivationBlock if that's ok, and will leave a note on the required name-change

@skeletor-spaceman skeletor-spaceman requested a review from tynes August 6, 2024 17:25
tynes
tynes reviewed Aug 9, 2024
View reviewed changes
op-node/rollup/derive/l1_block_info.go
return nil, fmt.Errorf("failed to marshal Isthmus l1 block info: %w", err)
}
return out, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no newline

Copy link
Collaborator Author

@skeletor-spaceman skeletor-spaceman Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@tynes not sure what you mean. do you want a new-line? the other set of 3 unmarshal... are all clumped together

@0xDiscotech 0xDiscotech mentioned this pull request Aug 12, 2024
Merged
0xDiscotech and others added 2 commits August 12, 2024 16:20
@0xDiscotech
feat: ban deposits interop (#11451)
5e0d64c 
* fix: contracts compiler errors

* feat: create some set l1 block values ithmus test

* refactor: not cross l2 inbox error

* chore: checkpoint debugging test

* fix: contracts compiler errors

* feat: create some set l1 block values ithmus test

* refactor: not cross l2 inbox error

* chore: checkpoint debugging test

* refactor: call public function

* fix: update is deposit var storage slot

* chore: update l2 cross inbox tests with the new is deposit check

* feat: add is deposit test

* refactor: set is deposit as an unstructured storage slot var

* feat: add missing natspec

* feat: update semver

* fix: stick to natspec standards

* chore: enhance natspec

* chore: underscore slot constant name

* Revert "chore: underscore slot constant name"

This reverts commit e162361.

* chore: enhance l1 block test

* refactor: move the new isthmus logic to the l1 block interop contract

* refactor: move the isthmus test logic to the l1 block test contract

* refactor: functions order

* chore: remove unused imports
@skeletor-spaceman
fix: typo
2c370f7
@0xDiscotech 0xDiscotech mentioned this pull request Aug 12, 2024
Merged
@0xDiscotech
feat: ban deposits interop (#11454)
da494fd 
* fix: contracts compiler errors

* feat: create some set l1 block values ithmus test

* refactor: not cross l2 inbox error

* chore: checkpoint debugging test

* fix: contracts compiler errors

* feat: create some set l1 block values ithmus test

* refactor: not cross l2 inbox error

* chore: checkpoint debugging test

* refactor: call public function

* fix: update is deposit var storage slot

* chore: update l2 cross inbox tests with the new is deposit check

* feat: add is deposit test

* refactor: set is deposit as an unstructured storage slot var

* feat: add missing natspec

* feat: update semver

* fix: stick to natspec standards

* chore: enhance natspec

* chore: underscore slot constant name

* Revert "chore: underscore slot constant name"

This reverts commit e162361.

* chore: enhance l1 block test

* refactor: move the new isthmus logic to the l1 block interop contract

* refactor: move the isthmus test logic to the l1 block test contract

* refactor: functions order

* chore: remove unused imports

* feat: benchmark set values ecotone and isthmus functions
skeletor-spaceman
skeletor-spaceman commented Aug 14, 2024
View reviewed changes
packages/contracts-bedrock/src/cannon/MIPS2.sol Outdated
Comment on lines 386 to 387
}
else if (syscall_no == sys.SYS_GET_AFFINITY) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why has the linter modified these lines?

skeletor-spaceman
skeletor-spaceman commented Aug 14, 2024
View reviewed changes
packages/contracts-bedrock/scripts/periphery/drippie/DrippieConfig.s.sol
Comment on lines +119 to +120
}
else {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same linting weird behavior

0xDiscotech and others added 4 commits August 14, 2024 18:59
@0xDiscotech
feat: ban deposits interop (#11481)
09c549c 
* fix: contracts compiler errors

* feat: create some set l1 block values ithmus test

* refactor: not cross l2 inbox error

* chore: checkpoint debugging test

* fix: contracts compiler errors

* feat: create some set l1 block values ithmus test

* refactor: not cross l2 inbox error

* chore: checkpoint debugging test

* refactor: call public function

* fix: update is deposit var storage slot

* chore: update l2 cross inbox tests with the new is deposit check

* feat: add is deposit test

* refactor: set is deposit as an unstructured storage slot var

* feat: add missing natspec

* feat: update semver

* fix: stick to natspec standards

* chore: enhance natspec

* chore: underscore slot constant name

* Revert "chore: underscore slot constant name"

This reverts commit e162361.

* chore: enhance l1 block test

* refactor: move the new isthmus logic to the l1 block interop contract

* refactor: move the isthmus test logic to the l1 block test contract

* refactor: functions order

* chore: remove unused imports

* feat: benchmark set values ecotone and isthmus functions

* refactor: rename from l1 block interop to isthmus

* feat: add benchmark test for deposits complete function

* chore: update gas snapshot

* feat: add warm and non warm benchmarks

* chore: delete unused pkg
@skeletor-spaceman
chore: some renames
bc562de
@skeletor-spaceman
chore: update gas limit of DepositsComplete
a8fd430
@skeletor-spaceman
chore: merged main
0a803e5
@skeletor-spaceman skeletor-spaceman mentioned this pull request Aug 16, 2024
Merged
skeletor-spaceman and others added 3 commits August 22, 2024 11:36
@skeletor-spaceman
feat: merged from upstream
f2d8fb8
@skeletor-spaceman
feat: ban deposits client tests (#11504)
ccf505d 
* feat: first pass at some tests

* fix: removed check

* feat: added a few more tests

* feat: added final tests on spec

* fix: missguiding name

* fix: move gas limit comment to the constant definition

* feat: added qol hardfork config

* fix: remove return and revamped tests

* fix: missing test qol revamp
@skeletor-spaceman
feat: merged from develop
a17e357
@skeletor-spaceman skeletor-spaceman mentioned this pull request Sep 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@semgrep-app semgrep-app[bot] semgrep-app[bot] left review comments

@protolambda protolambda protolambda left review comments

@tynes tynes tynes left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Interop: Ban Deposits that Trigger Executing Messages for Devnet
4 participants
@skeletor-spaceman @tynes @protolambda @0xDiscotech