Skip to content

Simplify nested ownership transfer #302

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

Merged
merged 3 commits into from
Apr 3, 2025

Conversation

jackchuma
Copy link
Contributor

No description provided.

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Apr 1, 2025

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@jackchuma jackchuma requested review from xenoliss and cbfyi April 2, 2025 12:52
henridevieux
henridevieux previously approved these changes Apr 2, 2025

# 7-of-10
NESTED_SIGNER_TO_ADD=
SAFE_B_OWNERS_ENCODED=
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not strongly opposed to this because abi encoded addresses are quite readable but I think it's important to keep the .env human readable.

xenoliss
xenoliss previously approved these changes Apr 3, 2025
@cb-heimdall cb-heimdall dismissed stale reviews from henridevieux and xenoliss April 3, 2025 08:26

Approved review 2736240471 from henridevieux is now dismissed due to new commit. Re-request for approval.

Copy link
Contributor

@xenoliss xenoliss left a comment

Choose a reason for hiding this comment

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

False alarm sorry!

@cb-heimdall
Copy link
Collaborator

Review Error for xenoliss @ 2025-04-03 09:08:17 UTC
User cannot review their own commit


// Adds `SAFE_A` and `SAFE_B` as owners to `OWNER_SAFE` and sets threshold to 2
// `SAFE_A` should have same owners as `OWNER_SAFE`
// `SAFE_A` should have same threshold as `OWNER_SAFE`
Copy link
Contributor

@xenoliss xenoliss Apr 3, 2025

Choose a reason for hiding this comment

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

Is there any reason we do not mention that SAFE_A should be 3o6 (or more generally even that SAFE_A must have the exact same config as OWNER_SAFE)?

##
.PHONY: deploy
deploy:
forge script --rpc-url $(L1_RPC_URL) DeploySafes --account testnet-admin --broadcast -vvvv
Copy link

Choose a reason for hiding this comment

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

--account testnet-admin intentional?

@jackchuma jackchuma merged commit 0bf6881 into main Apr 3, 2025
3 checks passed
@jackchuma jackchuma deleted the jack/simplify-nested-ownership-transfer branch April 3, 2025 19:37
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