Skip to content

Orchestrator Updates #207

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
Closed
wants to merge 10 commits into from
Closed

Orchestrator Updates #207

wants to merge 10 commits into from

Conversation

@aalavandhan
Copy link
Member

@aalavandhan aalavandhan commented Apr 2, 2021 •
edited
Loading

Fixes issues listed in #192

  • The orchestrator imports the entire UFragmentsPolicy.sol. The typical way to address this is to just import the relevant interface, instead of importing the full contract. This helps reduce the deployed contract size.
  • Use the revert message to indicate the index of the external call transaction which failed
  • Use non upgradable version of Ownable in The Orchestrator contract.

@aalavandhan aalavandhan requested review from ahnaguib, brandoniles and thegostep and removed request for brandoniles and thegostep April 2, 2021 17:13
@aalavandhan
Copy link
Member Author

aalavandhan commented Apr 2, 2021

@tedwu13 you can review this too

brandoniles
brandoniles reviewed Apr 2, 2021
View reviewed changes
contracts/Orchestrator.sol
@@ -1,53 +1,72 @@
pragma solidity 0.7.6;
// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity 0.8.2;
Copy link
Member

@brandoniles brandoniles Apr 2, 2021

Choose a reason for hiding this comment

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

0.8 is really tempting but... the latest version that slither recommends is 0.7.6:
https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-versions-of-solidity
(changed just a few days ago from 0.6.11)

Are there any features from 0.8 that you need?

Copy link
Member Author

@aalavandhan aalavandhan Apr 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I feel like keeping up with the latest version that openzeppelin contracts use is a good practice. They are up to 0.8x now

@thegostep thoughts?

contracts/Orchestrator.sol Outdated
constructor(address policy_) public {
Ownable.initialize(msg.sender);
policy = UFragmentsPolicy(policy_);
constructor(address policy_) {
Copy link
Member

@brandoniles brandoniles Apr 2, 2021

Choose a reason for hiding this comment

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

Need to call the base constructor too?

Copy link
Member Author

@aalavandhan aalavandhan Apr 2, 2021

Choose a reason for hiding this comment

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

not really required with newer versions.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol#L26

But will add it for the sake of being explicit

aalavandhan and others added 6 commits April 2, 2021 15:34
@aalavandhan aalavandhan changed the title Orchestrator V2 Orchestrator Updates Apr 5, 2021
@aalavandhan aalavandhan force-pushed the orch-v2 branch 2 times, most recently from 10ebf06 to 542d176 Compare April 5, 2021 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brandoniles brandoniles brandoniles left review comments

@thegostep thegostep Awaiting requested review from thegostep

@ahnaguib ahnaguib Awaiting requested review from ahnaguib

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

@aalavandhan @brandoniles