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

feat(contracts): integrate token-based permissions #201

Merged
merged 1 commit into from
Sep 27, 2023
Merged

feat(contracts): integrate token-based permissions #201

merged 1 commit into from
Sep 27, 2023

Conversation

kangsorang
Copy link
Contributor

Description

Modified the membership of the security council, which was managed as a separate list, to be based on token.

@kangsorang kangsorang requested a review from a team as a code owner September 21, 2023 03:01
0xHansLee
0xHansLee reviewed Sep 21, 2023
View reviewed changes
Copy link
Contributor

@0xHansLee 0xHansLee left a comment

Choose a reason for hiding this comment

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

Please update gas-snapshot and storage-layout.
And why don't we merge two contract files (SecurityCouncil.sol and SimpleMultiSigWallet.sol)?

packages/contracts/contracts/L1/SecurityCouncil.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/L1/SecurityCouncil.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/ISimpleMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/SimpleMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/SimpleMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/SimpleMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/libraries/Types.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/L1/SecurityCouncil.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/SimpleMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/L1/SecurityCouncil.sol Show resolved Hide resolved
@kangsorang kangsorang force-pushed the feat/integration-token-based-permissions branch 4 times, most recently from 377acb5 to f2e6c35 Compare September 22, 2023 03:15
0xHansLee
0xHansLee reviewed Sep 23, 2023
View reviewed changes
packages/contracts/contracts/universal/TokenMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/ISimpleMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/KromaSoulBoundERC721.sol Show resolved Hide resolved
utils/chain-ops/genesis/layer_one.go Show resolved Hide resolved
packages/contracts/contracts/test/CommonTest.t.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/test/CommonTest.t.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/test/SecurityCouncil.t.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/test/SecurityCouncil.t.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/test/SecurityCouncil.t.sol Show resolved Hide resolved
seolaoh
seolaoh reviewed Sep 25, 2023
View reviewed changes
packages/contracts/contracts/universal/ITokenMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/ITokenMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/ITokenMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/ITokenMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/TokenMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/deploy/L1/016-SecurityCouncil.ts Outdated Show resolved Hide resolved
e2e/actions/l2_runtime.go Outdated Show resolved Hide resolved
packages/contracts/contracts/test/SecurityCouncil.t.sol Outdated Show resolved Hide resolved
e2e/e2eutils/setup.go Outdated Show resolved Hide resolved
utils/chain-ops/genesis/testdata/test-deploy-config-devnet-l1.json Outdated Show resolved Hide resolved
@seolaoh
Copy link
Contributor

seolaoh commented Sep 25, 2023

Additionally, we have a commit scope named contracts, not contract. Please modify it in PR title and commit msg.
Also we have agreed to start the branch name with a verb like feat/intergrate-token-based-permissions, please follow it next time!

@kangsorang kangsorang force-pushed the feat/integration-token-based-permissions branch from f2e6c35 to cad1775 Compare September 26, 2023 04:06
@kangsorang kangsorang changed the title feat(contract): integration token-based permissions feat(contracts): integrate token-based permissions Sep 26, 2023
@kangsorang kangsorang force-pushed the feat/integration-token-based-permissions branch 5 times, most recently from 4525a28 to ab37049 Compare September 26, 2023 05:07
seolaoh
seolaoh reviewed Sep 26, 2023
View reviewed changes
packages/contracts/contracts/test/SecurityCouncil.t.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/ITokenMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/TokenMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/TokenMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/TokenMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/TokenMultiSigWallet.sol Show resolved Hide resolved
packages/contracts/contracts/universal/ITokenMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/ITokenMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/TokenMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/ISimpleMultiSigWallet.sol Outdated Show resolved Hide resolved
@kangsorang kangsorang force-pushed the feat/integration-token-based-permissions branch 2 times, most recently from 7be63e0 to f9b2b34 Compare September 26, 2023 07:49
@kangsorang kangsorang requested a review from seolaoh September 26, 2023 07:50
@kangsorang kangsorang force-pushed the feat/integration-token-based-permissions branch from f9b2b34 to 824bd29 Compare September 26, 2023 09:17
@kangsorang kangsorang requested a review from seolaoh September 26, 2023 09:17
0xHansLee
0xHansLee reviewed Sep 26, 2023
View reviewed changes
Copy link
Contributor

@0xHansLee 0xHansLee left a comment

Choose a reason for hiding this comment

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

Please update the semver of KromaSoulBoundERC721.sol to v1.0.1.

packages/contracts/contracts/universal/ITokenMultiSigWallet.sol Outdated Show resolved Hide resolved
packages/contracts/contracts/universal/TokenMultiSigWallet.sol Show resolved Hide resolved
@kangsorang kangsorang force-pushed the feat/integration-token-based-permissions branch from 824bd29 to afda914 Compare September 27, 2023 02:34
@kangsorang kangsorang force-pushed the feat/integration-token-based-permissions branch from afda914 to 91fca42 Compare September 27, 2023 05:35
@kangsorang kangsorang force-pushed the feat/integration-token-based-permissions branch from 91fca42 to db991ad Compare September 27, 2023 05:46
0xHansLee
0xHansLee approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@0xHansLee 0xHansLee left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@kangsorang kangsorang merged commit adb2ae6 into dev Sep 27, 2023
@kangsorang kangsorang deleted the feat/integration-token-based-permissions branch September 27, 2023 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seolaoh seolaoh seolaoh approved these changes

@0xHansLee 0xHansLee 0xHansLee approved these changes

@Pangssu Pangssu Pangssu approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@kangsorang @seolaoh @Pangssu @0xHansLee