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

Update base-org/contracts dependency #356

Merged
merged 6 commits into from
Nov 4, 2024
Merged

Conversation

mdehoog
Copy link
Contributor

@mdehoog mdehoog commented Oct 29, 2024

Description

We did a big cleanup of the *MultisigBuilder contracts in base-org/contracts#97.

This PR bumps the base-org/contracts dependency to pick up these changes, and makes all the required updates for the scripts.

Tests

I tested one of the tasks before and after this change to ensure that the same data-to-sign was produced, as well as the same output / assertions.

@mdehoog mdehoog force-pushed the michael/update-base-contracts branch from b455e57 to 9dd4e2a Compare October 29, 2024 01:43
@mdehoog mdehoog marked this pull request as ready for review October 29, 2024 01:45
@mdehoog mdehoog requested review from a team as code owners October 29, 2024 01:45
@mdehoog mdehoog requested a review from mds1 October 29, 2024 01:45
@mdehoog
Copy link
Contributor Author

mdehoog commented Oct 29, 2024

This can be simplified with base-org/contracts#103... converting to draft until this is merged

@mdehoog mdehoog marked this pull request as draft October 29, 2024 18:15
@mdehoog mdehoog force-pushed the michael/update-base-contracts branch from 4457df7 to 2f2f1d1 Compare October 29, 2024 21:23
@mdehoog mdehoog marked this pull request as ready for review October 29, 2024 21:25
mds1
mds1 previously approved these changes Oct 31, 2024
Copy link
Contributor

@mds1 mds1 left a comment

Choose a reason for hiding this comment

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

This looks great, and the upstream base-org/contracts change are really nice. Thank you for this!

I tested one of the tasks before and after this change to ensure that the same data-to-sign was produced, as well as the same output / assertions.

Before merging, @Ethnical can you:

I don't expect any issues with any of the above, but just want to be sure before we merge

Copy link
Contributor

@mds1 mds1 left a comment

Choose a reason for hiding this comment

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

I've confirmed all the above tasks, so will merge

@mdehoog One thing I noticed is that the state overrides in the nested case seem to be different—we no longer update the Safe owners to be a single owner of the Multicall3 contract. Was this intentional, and if so what is the new approach for nested overrides?

@mds1 mds1 added this pull request to the merge queue Nov 4, 2024
Merged via the queue into main with commit f796091 Nov 4, 2024
14 checks passed
@mds1 mds1 deleted the michael/update-base-contracts branch November 4, 2024 22:25
@mdehoog
Copy link
Contributor Author

mdehoog commented Nov 4, 2024

thanks for the review @mds1!

I didn't intentionally update the simulation overrides, but I believe the owner=multicall override is still present?
https://github.com/base-org/contracts/blob/12afb9f603bfe20a59dd772ae9bb9a8e68e14d45/script/universal/NestedMultisigBuilder.sol#L220

@mds1
Copy link
Contributor

mds1 commented Nov 4, 2024

I noticed differently tenderly URLs when comparing the 018-granite-upgrade task on main vs. this branch: Here is a diff, the right side is the new URL: https://difff.jp/en/eyse3.html

And corresponding screenshot of the tenderly UI when loading that URL
image

The issue here might be related to the fact that the task is already executed and now effective has a no-op state diff, so I did not investigate this much

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