-
Notifications
You must be signed in to change notification settings - Fork 246
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
Kill switch: Polish and integrate MVP version #518
Draft
facuspagnuolo
wants to merge
38
commits into
next
Choose a base branch
from
application_kill_switch
base: next
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 14 commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
383dd79
contracts: base kill switch implementation
facuspagnuolo 9d7a118
contracts: binary kill switch implementation
facuspagnuolo c3f78fd
contracts: severities kill switch implementation
facuspagnuolo 4d9ce01
contracts: application level kill switch
facuspagnuolo 51127c4
contracts: kernel level kill switch
facuspagnuolo b75fdca
tests: add kill switch unit tests
facuspagnuolo 4f4421e
contracts: use ternary ignoration for kill-switch settings
facuspagnuolo 00b76b9
contracts: fix linting issues
facuspagnuolo df3568f
kill-switch: cleanup models leaving only app level
facuspagnuolo b6c5531
kill-switch: use `highest` instead of `lowest` allowed severities
facuspagnuolo c20c608
kill-switch: Support one issues registry per contract
facuspagnuolo 1f17dcb
kill-switch: drop custom programmatic handling
facuspagnuolo 130c343
kill-switch: integrate with aragonOS components
facuspagnuolo 3c9eb50
kill-switch: place mocks and test files in corresponding dirs
facuspagnuolo ffff170
kill-switch: minor tweaks based on @bingen's feedback
facuspagnuolo 76f8521
kill-switch: skip gas costs test for coverage
facuspagnuolo 2db3e46
kill-switch: rename 'ignore' action to 'allow'
facuspagnuolo d28cfee
kill-switch: rename issues registry `entry` by `implementation`
facuspagnuolo dba7e42
kill-switch: rename test files to follow naming convention
facuspagnuolo 60268b2
kill-switch: improve authentication params
facuspagnuolo 8f91fec
kill-switch: fix app id constant value
facuspagnuolo 1ce842d
kill-switch: improve gas costs and support current kernel versions
facuspagnuolo a13d491
kill-switch: move kernel initialization logic to DAO factory
facuspagnuolo 77b8b2e
kill-switch: rename issues registry `isSeverityFor` to `hasSeverity`
facuspagnuolo d499267
kill-switch: rename `killSwitched` modifier to `killSwitchProtected`
facuspagnuolo 17922cc
kill-switch: improve settings to customize different scenarios
facuspagnuolo 0f288ce
kill-switch: add `DAOFactory` tests
facuspagnuolo a5f6bfe
kill-switch: test non compliant kernel versions
facuspagnuolo 7f73ac3
kill-switch: allow core instances by default
facuspagnuolo 5230a86
kill-switch: optimize tests
facuspagnuolo fd6348c
kill-switch: parameterize instance being called in kernel
facuspagnuolo 6ab5f69
Merge branch 'dev' of github.com:aragon/aragonOS into application_kil…
facuspagnuolo ae617f6
kill-switch: apply suggestions from @izqui
facuspagnuolo ce7a29b
kill-switch: extract `killSwitchProtected` modifier to function
facuspagnuolo 08f1a8b
kill-switch: address feedback from @sohkai
facuspagnuolo 5817661
kill-switch: small tweaks and optimizations
facuspagnuolo 3113233
kill-switch: split unit and integration tests
facuspagnuolo c057f0d
kill-switch: rename mocks dir and improve inline doc
facuspagnuolo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
pragma solidity 0.4.24; | ||
facuspagnuolo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
contract IIssuesRegistry { | ||
enum Severity { None, Low, Mid, High, Critical } | ||
|
||
event SeveritySet(address indexed entry, Severity severity, address sender); | ||
facuspagnuolo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
function initialize() external; | ||
|
||
function setSeverityFor(address entry, Severity severity) external; | ||
|
||
function isSeverityFor(address entry) public view returns (bool); | ||
|
||
function getSeverityFor(address entry) public view returns (Severity); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
pragma solidity 0.4.24; | ||
|
||
import "./IIssuesRegistry.sol"; | ||
|
||
|
||
contract IKillSwitch { | ||
function initialize(IIssuesRegistry _issuesRegistry) external; | ||
facuspagnuolo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
function shouldDenyCallingContract(address _contract) external returns (bool); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
pragma solidity 0.4.24; | ||
|
||
import "../apps/AragonApp.sol"; | ||
import "./IIssuesRegistry.sol"; | ||
|
||
|
||
contract IssuesRegistry is IIssuesRegistry, AragonApp { | ||
bytes32 constant public SET_ENTRY_SEVERITY_ROLE = keccak256("SET_ENTRY_SEVERITY_ROLE"); | ||
|
||
mapping (address => Severity) internal issuesSeverity; | ||
|
||
function initialize() external onlyInit { | ||
initialized(); | ||
} | ||
|
||
function setSeverityFor(address entry, Severity severity) external authP(SET_ENTRY_SEVERITY_ROLE, arr(entry, msg.sender)) { | ||
facuspagnuolo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
issuesSeverity[entry] = severity; | ||
facuspagnuolo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
emit SeveritySet(entry, severity, msg.sender); | ||
} | ||
|
||
function isSeverityFor(address entry) public view isInitialized returns (bool) { | ||
facuspagnuolo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return issuesSeverity[entry] != Severity.None; | ||
} | ||
|
||
function getSeverityFor(address entry) public view isInitialized returns (Severity) { | ||
return issuesSeverity[entry]; | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should directly bundle this into
auth()
or something similar as well. It may be tedious for developers to place this on every external, state-changing call (similar toisInitialized()
; this is my envisioned recommendation for apps but correct me if you think devs should have more granularity).We may also want to find a better name, e.g.
killSwitchProtected
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it may make sense but before coupling them we can keep working on other details and leave that decision for later. We will probably have a better understanding of how the whole kill-switch thing interacts with a DAO once we have finished some of the pending stuff.
OTOH, I like the new name you propose :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, makes sense, although I wonder if, given the gas overhead, it could be useful to have that granularity to only apply the killSwitch to very critical functions.
Nevertheless, one could make the argue that if the function has an ACL protection is likely critical. 🤔