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

[Internal audit] aragonOS 5 #612

Merged
merged 14 commits into from
Aug 18, 2020
Merged

[Internal audit] aragonOS 5 #612

merged 14 commits into from
Aug 18, 2020

Conversation

sohkai
Copy link
Contributor

@sohkai sohkai commented Aug 11, 2020

Includes a number of small changes made during the audit, easiest to review commit by commit.

The only contract change:

  • IArbitrable: aligns its ERC165 implementation with the other contracts

@sohkai sohkai requested a review from facuspagnuolo August 11, 2020 18:59
@auto-assign auto-assign bot requested a review from izqui August 11, 2020 19:00


// Note
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is my primary note.

I've started thinking about the IAgreement as a "disputable action registry" (IDisputableActionRegistry), and backtracking on whether we need everything in this interface.

From a Disputable's standpoint, I think the main things its "action registry" should care about are:

  • Registering new actions, which may be disputable
  • Challenging an action
  • Closing old actions
  • Getting a little bit of information about a particular action or challenge

With that in mind, I started thinking that specific implementation-specific information about the "action registry" would be reduced and queried separately from the action disputable's information (e.g. when using subgraphs). For example, I noticed you only used the Agreement to find the vote's ID when disputes were created in the DisputableVoting subgraph.

Copy link
Contributor

Choose a reason for hiding this comment

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

We decided not to force the IArbitrable and IACL inheritance at this level. The rest of the changes will be included in the aragonOS v6 roadmap

contracts/apps/disputable/DisputableAragonApp.sol Outdated Show resolved Hide resolved
contracts/apps/disputable/DisputableAragonApp.sol Outdated Show resolved Hide resolved
contracts/forwarding/IAbstractForwarder.sol Show resolved Hide resolved
contracts/lib/arbitration/IAragonAppFeesCashier.sol Outdated Show resolved Hide resolved
contracts/lib/arbitration/IArbitrable.sol Outdated Show resolved Hide resolved
contracts/test/mocks/apps/disputable/AgreementMock.sol Outdated Show resolved Hide resolved
test/contracts/factory/evm_script_factory.js Outdated Show resolved Hide resolved
@facuspagnuolo
Copy link
Contributor

Thx for the audit @sohkai! I addressed your comments commit per commit, if it looks good we can release a new version and update the agreement and disputable voting apps

Copy link
Contributor Author

@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.

Last few things, I'm still wondering if we should eliminate more from IAgreement 🔪

contracts/apps/disputable/IAgreement.sol Outdated Show resolved Hide resolved
contracts/apps/disputable/IAgreement.sol Outdated Show resolved Hide resolved
contracts/lib/arbitration/IAragonAppFeesCashier.sol Outdated Show resolved Hide resolved
@facuspagnuolo
Copy link
Contributor

@sohkai I like the idea! I addressed your comments, let me know your thoughts. About the app fees cashier inline doc, I had actually brought the content from the court repo, so I think we are fine :)

Copy link
Contributor Author

@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.

💃🕺 Let's merge!

@facuspagnuolo facuspagnuolo merged commit d09883c into next Aug 18, 2020
@facuspagnuolo facuspagnuolo deleted the internal-audit branch August 18, 2020 18:22
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.

2 participants