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

Linear erc20 swap offer enforcer #26

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

danfinlay
Copy link
Contributor

Allows offering an amount of an ERC20 in exchange for any number of partial orders of another ERC20 at a given rate.

This is my current best draft of a preprompt to help development of caveat enforcers.

It will need to be kept up to date when we change interfaces. I believe I have it up to date with main now.
@danfinlay danfinlay requested a review from a team as a code owner October 2, 2024 17:23
bytes4 selector = bytes4(callData_[0:4]);
require(selector == IERC20.transfer.selector || selector == IERC20.transferFrom.selector, "SwapOfferEnforcer:invalid-method");

uint256 amount = uint256(bytes32(callData_[36:68]));
Copy link
Contributor

Choose a reason for hiding this comment

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

The amount should be extracted from different ranges if it is transfer [36:68] or transferFrom [68:100]

Comment on lines +186 to +190
require(offer.tokenIn == terms.tokenIn && offer.tokenOut == terms.tokenOut &&
offer.amountIn == terms.amountIn && offer.amountOut == terms.amountOut &&
offer.recipient == terms.recipient,
"SwapOfferEnforcer:terms-mismatch");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it would be more expensive to read it like this rather than to create a copy in memory.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to have a test for erc20.transferFrom


struct SwapOfferArgs {
uint256 claimedAmount;
IDelegationManager delegationManager;
Copy link
Contributor

Choose a reason for hiding this comment

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

It is safer not to allow the redeemer to provide the delegationManager address in the args, instead always use msg.sender as the other enforcers. As it is right now someone could call the beforeHook directly and modify values that he shouldn't.

override
onlySingleExecutionMode(_mode)
{
SwapOfferArgs memory args = abi.decode(_args, (SwapOfferArgs));
Copy link
Contributor

Choose a reason for hiding this comment

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

Underscore in variable names


require(offer.amountOutFilled + amount <= offer.amountOut, "SwapOfferEnforcer:exceeds-output-amount");

amountInFilled_ = offer.amountInFilled + _claimedAmount;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why we read these offer.amountInFilled and offer.amountOutFilled instead of resetting them to 0 if the offer already existed

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