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

refactor: add BaseExecutor with built in permit #202

Closed
wants to merge 45 commits into from

Conversation

zhongeric
Copy link
Collaborator

@zhongeric zhongeric commented Sep 12, 2023

SwapRouter02Executor now inherits BaseExecutor which adds a new entrypoint multicall which allows callers to chain together series of execute and permit calls.

@zhongeric zhongeric changed the title Add base executor refactor: add BaseExecutor with built in permit Sep 12, 2023
src/sample-executors/BaseExecutor.sol Outdated Show resolved Hide resolved
src/sample-executors/BaseExecutor.sol Outdated Show resolved Hide resolved
src/sample-executors/BaseExecutor.sol Outdated Show resolved Hide resolved
src/sample-executors/BaseExecutor.sol Outdated Show resolved Hide resolved

function reactorCallback(ResolvedOrder[] calldata, bytes calldata callbackData) external virtual;

function execute(SignedOrder memory order, bytes memory callbackData) external payable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should these be onlyOwner?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mmm i thought we wanted execute to be gated by onlyWhitelstedCaller?

src/sample-executors/BaseExecutor.sol Show resolved Hide resolved
/// @inheritdoc IReactorCallback
function reactorCallback(ResolvedOrder[] calldata, bytes calldata callbackData) external virtual;

function execute(SignedOrder memory order, bytes memory callbackData) public payable virtual {
Copy link
Member

Choose a reason for hiding this comment

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

why is this virtual?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

made this virtual so we can add the onlyWhitelistedCaller modifier to it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not a huge fan of this auth scheme, especially for reactorCallback because that must be restricted to only being called by the reactor but since its virtual in this base executor we can't enforce that here. So it's possible that integrators could make the mistake of not gating it by caller

Copy link
Member

Choose a reason for hiding this comment

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

yep, discussed offline and we should remove impl here and just override in inheriting contract

src/sample-executors/BaseExecutor.sol Outdated Show resolved Hide resolved
src/sample-executors/BaseExecutor.sol Outdated Show resolved Hide resolved
src/sample-executors/BaseExecutor.sol Outdated Show resolved Hide resolved
src/sample-executors/BaseExecutor.sol Outdated Show resolved Hide resolved
Comment on lines +29 to +31
/// @inheritdoc IReactorCallback
/// @dev any overriding function MUST use the onlyReactor modifier
function reactorCallback(ResolvedOrder[] calldata, bytes calldata) external virtual onlyReactor {}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since solidity doesn't require an overriding function to use the same modifier, I'm tempted to have this base contract not inherit the IReactor interface entirely and leave that up to integrations

@marktoda
Copy link
Collaborator

closing as current preference is for fillers to do this in their fill contract

@marktoda marktoda closed this Dec 12, 2023
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.

4 participants