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

Problem: control admin in the solidity code is not flexible #195

Open
wants to merge 1 commit into
base: v2.0.0-cronos
Choose a base branch
from
Open

Problem: control admin in the solidity code is not flexible #195

wants to merge 1 commit into from

Conversation

thomas-nguy
Copy link
Collaborator

@thomas-nguy thomas-nguy commented Jan 12, 2023 •
edited
Loading

Currently only the owner can pause/unpause the contract.

We want to make it more flexible so that only some special addresses can have access to those functionalities
( needed for the circuit breaker)

@thomas-nguy thomas-nguy force-pushed the thomas/gravity-contract-flexible-admins branch from 533756d to 241e2f0 Compare January 12, 2023 05:18
@thomas-nguy thomas-nguy temporarily deployed to CI January 12, 2023 05:40 — with GitHub Actions Inactive
@thomas-nguy thomas-nguy temporarily deployed to CI January 12, 2023 05:40 — with GitHub Actions Inactive
@thomas-nguy thomas-nguy temporarily deployed to CI January 12, 2023 05:40 — with GitHub Actions Inactive
@thomas-nguy thomas-nguy temporarily deployed to CI January 12, 2023 05:40 — with GitHub Actions Inactive
@FinnZhangCrypto
Copy link
Collaborator

FinnZhangCrypto commented Jan 17, 2023 •
edited
Loading

Checked the new changes on admin roles. Looks good to me. See if @brianatcrypto have any comments?
There is one question from my side, in production environment, the whitelist access of deployERC20 is only can be grated to token project side and our side right? No external side can get this access right?

@thomas-nguy
Copy link
Collaborator Author

Yes, I have added the whitelist guard to deployERC20

Whitelist are addresses that are able to relay (submit batches) but we can extend it now to also those who can deploy cosmos erc20.

At anytime, the whitelist can be turned off by setting anyoneCanRelay to true

I think it can add an extra security to prevent anyone to call this method (from recent bug) and since the address mapping in cronos is already permissioned, there is no use to make it public

@FinnZhangCrypto
Copy link
Collaborator

FinnZhangCrypto commented Jan 17, 2023 •
edited
Loading

Thanks @thomas-nguy
The logic of switching on/off whitList makes sence to me!
And the maliciously deployed CosmosERC20 token cannot be recognized/acknowledged forever right?

FinnZhangCrypto
FinnZhangCrypto approved these changes Jan 18, 2023
View reviewed changes
Copy link
Collaborator

@FinnZhangCrypto FinnZhangCrypto left a comment

Choose a reason for hiding this comment

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

Looks good to me.

calvinaco
calvinaco reviewed Jan 27, 2023
View reviewed changes
Copy link

@calvinaco calvinaco left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM but I will leave solidity expertise to approve the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calvinaco calvinaco calvinaco left review comments

@FinnZhangCrypto FinnZhangCrypto FinnZhangCrypto approved these changes

@brianatcrypto brianatcrypto Awaiting requested review from brianatcrypto

@John-decrypt John-decrypt Awaiting requested review from John-decrypt

@cyril-crypto cyril-crypto Awaiting requested review from cyril-crypto

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

Successfully merging this pull request may close these issues.
3 participants
@thomas-nguy @FinnZhangCrypto @calvinaco