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

Kill switch: Polish and integrate MVP version #518

Draft
wants to merge 38 commits into
base: next
Choose a base branch
from

Conversation

facuspagnuolo
Copy link
Contributor

@facuspagnuolo facuspagnuolo commented Apr 30, 2019

Follow up on #516

Based on an offline discussion we had about the next-steps regarding the kill-switch topic, we decided to:

  • Drop the kernel level kill-switch idea, at least for the MVP version
  • Simplify the whole architecture to support only application level kill-switch using severities
  • Support having one issue registry per application
  • Integrate with the rest of the aragonOS components
  • Investigate how an organization is able to recover

Summing up, the new contracts model looks like:
image

And the contracts interaction is the following:
image

@facuspagnuolo facuspagnuolo changed the title [WIP] Application kill switch [WIP] Application level kill switch Apr 30, 2019
@facuspagnuolo facuspagnuolo force-pushed the application_kill_switch branch from 1f702fa to ff8393d Compare April 30, 2019 19:19
@aragon aragon deleted a comment from coveralls Apr 30, 2019
@facuspagnuolo facuspagnuolo self-assigned this May 1, 2019
@facuspagnuolo facuspagnuolo changed the title [WIP] Application level kill switch Kill switch - Polish and integrate MVP version May 1, 2019
@facuspagnuolo facuspagnuolo removed the wip label May 1, 2019
@facuspagnuolo facuspagnuolo changed the title Kill switch - Polish and integrate MVP version Kill switch: Polish and integrate MVP version May 1, 2019
@facuspagnuolo facuspagnuolo force-pushed the application_kill_switch branch 2 times, most recently from 9eda76e to d23363d Compare May 1, 2019 11:57
@facuspagnuolo facuspagnuolo changed the base branch from kill_switch_poc to master May 1, 2019 12:04
@facuspagnuolo facuspagnuolo changed the base branch from master to dev May 1, 2019 12:05
contracts/apps/AragonApp.sol Outdated Show resolved Hide resolved
contracts/apps/AragonApp.sol Outdated Show resolved Hide resolved
contracts/apps/AragonApp.sol Outdated Show resolved Hide resolved
contracts/apps/AragonApp.sol Outdated Show resolved Hide resolved
contracts/kernel/IKernel.sol Outdated Show resolved Hide resolved
contracts/kernel/Kernel.sol Outdated Show resolved Hide resolved
contracts/factory/DAOFactory.sol Outdated Show resolved Hide resolved
contracts/factory/DAOFactory.sol Outdated Show resolved Hide resolved
contracts/kill_switch/KillSwitch.sol Outdated Show resolved Hide resolved
contracts/kill_switch/KillSwitch.sol Outdated Show resolved Hide resolved
Co-Authored-By: Jorge Izquierdo <izqui97@gmail.com>
@facuspagnuolo facuspagnuolo force-pushed the application_kill_switch branch from ff52bb7 to ae617f6 Compare May 17, 2019 16:50
Copy link
Contributor

@sohkai sohkai left a comment

Choose a reason for hiding this comment

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

There's a bunch of really small comments in this review, but the main ones I'd like to bring up:

  • Making the interface a bit more generic; rather than constraining it to just the kill switch perhaps we can operate on the level of an app being "enabled" or "disabled" by external factors
  • Perhaps we can make the check for the KillSwitch a view interface, so we can use staticcalls
  • Again, rolling up the kill switch detection in AragonApp into canPerform(); to me this fits a lot better with the disabled / enabled model of an app
  • Simplifying DAOFactory; I really liked how simple the old one was 😄

contracts/apps/AragonApp.sol Outdated Show resolved Hide resolved
contracts/apps/AragonApp.sol Outdated Show resolved Hide resolved
contracts/apps/AragonApp.sol Outdated Show resolved Hide resolved
contracts/apps/AragonApp.sol Outdated Show resolved Hide resolved
contracts/apps/AragonApp.sol Outdated Show resolved Hide resolved
contracts/kill_switch/KillSwitch.sol Outdated Show resolved Hide resolved
contracts/kill_switch/KillSwitch.sol Outdated Show resolved Hide resolved
contracts/test/mocks/kill_switch/FailingKillSwitchMock.sol Outdated Show resolved Hide resolved
test/contracts/kill_switch/issues_registry.test.js Outdated Show resolved Hide resolved
test/contracts/kill_switch/issues_registry.test.js Outdated Show resolved Hide resolved
@facuspagnuolo
Copy link
Contributor Author

Thanks for the review @sohkai! I already addressed/answered all your comments. Feel free to take another look if you want.

contracts/apps/AragonApp.sol Outdated Show resolved Hide resolved
contracts/kernel/Kernel.sol Outdated Show resolved Hide resolved
contracts/kill-switch/KillSwitch.sol Outdated Show resolved Hide resolved
test/contracts/kill-switch/kill_switch.js Outdated Show resolved Hide resolved
@facuspagnuolo
Copy link
Contributor Author

The coverage task is failing because there are some tests that fail but only when measuring coverage. I've trying to debug it locally but it was completely cumbersome and I couldn't get to know what was going on. After that, I tried running solidity-coverage on using ganache-cli instead of testrpc-sc using frangio's fork and it passed. Definitely solidity-coverage is an outdated tool that has already been giving us several headaches 😕

Copy link
Contributor

@sohkai sohkai left a comment

Choose a reason for hiding this comment

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

🎉 This is looking great; not sure how we should work around the coverage task for now (perhaps we can even just install frangio's fork 😄).

One thing I was looking into earlier (and unfortunately dropped) was doing some investigation into why the gas overhead was so high for checking the killswitch (27k without a SSTORE is quite high!). It'd be nice to see if there was anything obvious to make this a bit lower.

@sohkai sohkai changed the base branch from dev to next July 11, 2019 09:30
@sohkai sohkai marked this pull request as draft June 4, 2020 10:30
@sohkai sohkai added this to the aragonOS 6.0 milestone Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants