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

Orchestrator V2 #192

Open
2 of 6 tasks
aalavandhan opened this issue Jan 28, 2021 · 3 comments · Fixed by #203 or #204
Open
2 of 6 tasks

Orchestrator V2 #192

aalavandhan opened this issue Jan 28, 2021 · 3 comments · Fixed by #203 or #204
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@aalavandhan
Copy link
Member

aalavandhan commented Jan 28, 2021

Contributors can pick up of the issues listed here (all are good first issues, great to get started):

  • The Orchestrator was created to disregard failed transactions. However it was patched to revert if one of the listed transactions fail (to prevent a gas underprice attack). Update the inline code documentation to reflect the patched change].

  • Replace externalCall with .call

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

  • The current Orchestrator contract uses require(msg.sender == tx.origin) to prevent contracts from calling rebase. But tx.origin may be deprecated in the future. Implement one of these alternate mechanisms described here.

@aalavandhan aalavandhan added help wanted Extra attention is needed good first issue Good for newcomers labels Jan 28, 2021
@aalavandhan aalavandhan pinned this issue Jan 28, 2021
@aalavandhan aalavandhan changed the title HELP WANTED - Orchestrator V2 Orchestrator V2 Jan 28, 2021
@aalavandhan aalavandhan mentioned this issue Feb 8, 2021
@tedwu13
Copy link
Contributor

tedwu13 commented Feb 18, 2021

@nithinkrishna Can you explain a little more about Replace externalCall with .call? I see that externalCall function already has a .call function wrapped inside of the inline assembly.

@aalavandhan
Copy link
Member Author

aalavandhan commented Feb 18, 2021

@tedwu13 With solidity 0.4.24 we had to use assembly to make external calls. With later versions you can use address.call(data) to make an external function call.

The idea is to get rid of the externalCall method we've defined and just use the native solidity method.

https://docs.soliditylang.org/en/v0.7.0/control-structures.html?highlight=call#external-function-calls

# something like?
Transaction storage t = transactions[i];
(bool result, bytes memory reason) = t.destination.call(t.data);

@aalavandhan aalavandhan linked a pull request Feb 19, 2021 that will close this issue
2 tasks
@aalavandhan aalavandhan reopened this Feb 19, 2021
@baribari1
Copy link

baribari1 commented Mar 30, 2021

@nithinkrishna I think tx.origin is here to stay in order to protect backwards compatibility - see below.
ethereum/solidity#683

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
3 participants