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

contracts-bedrock: cleanup FeeVault #12338

Merged
merged 7 commits into from
Oct 7, 2024
Merged

Conversation

tynes
Copy link
Contributor

@tynes tynes commented Oct 5, 2024

Description

Updates the FeeVault to follow modern conventions used in the repo
by moving to usage of interfaces rather than implementations. Also
moves the FeeVault into the L2 package as its only really useful
on L2. This is meant to reduce the diff for the Stanard L2 Genesis
by breaking up the refactor into its own small PR.

Updates the `FeeVault` to follow modern conventions used in the repo
by moving to usage of interfaces rather than implementations. Also
moves the `FeeVault` into the `L2` package as its only really useful
on L2. This is meant to reduce the diff for the Stanard L2 Genesis
by breaking up the refactor into its own small PR.
@tynes tynes requested a review from a team as a code owner October 5, 2024 00:30
@tynes tynes requested a review from clabby October 5, 2024 00:30
Copy link
Contributor

semgrep-app bot commented Oct 5, 2024

Semgrep found 1 sol-style-doc-comment finding:

  • packages/contracts-bedrock/src/safe/SafeSigners.sol

Javadoc-style comments are not allowed. Use /// style doc comments instead.

Ignore this finding from sol-style-doc-comment.

There is an issue with normalization of enums when they are return
values
@tynes
Copy link
Contributor Author

tynes commented Oct 6, 2024

@smartcontracts I added the fee vaults to the ignore list in the interface script.

They were giving the following error:

       {
         "name": "_withdrawalNetwork",
         "type": "uint8",
-        "internalType": "Types.WithdrawalNetwork"
+        "internalType": "enum Types.WithdrawalNetwork"
       }

This requires normalization, which given you are rewriting this script in Go, I didn't want to duplicate the work. This issue arises even if the WithdrawalNetwork is defined in the FeeVault, since the interface autogen with cast creates a library specific to the interface director when the enum is defined outside of the interface.

I recommend we think a bit from first principles rather than just staying within the confines of cast's behavior, ie we could ask for an update to cast interface or directly generate the interface files with our own tooling

@tynes tynes enabled auto-merge October 7, 2024 16:47
@tynes tynes added this pull request to the merge queue Oct 7, 2024
Merged via the queue into develop with commit b460aa2 Oct 7, 2024
61 checks passed
@tynes tynes deleted the contracts/fee-vault-cleanup branch October 7, 2024 17:37
samlaf pushed a commit to samlaf/optimism that referenced this pull request Nov 10, 2024
* contracts-bedrock: cleanup `FeeVault`

Updates the `FeeVault` to follow modern conventions used in the repo
by moving to usage of interfaces rather than implementations. Also
moves the `FeeVault` into the `L2` package as its only really useful
on L2. This is meant to reduce the diff for the Stanard L2 Genesis
by breaking up the refactor into its own small PR.

* contracts: update semver-lock

* semver-lock: fixup

* cleanup: refactor

* lint: fix

* snapshots: regenerate

* interface check: ignore fee vaults

There is an issue with normalization of enums when they are return
values
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.

2 participants