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

feat: permit2 for token flows #63

Merged
merged 19 commits into from
Jul 20, 2024
Merged

feat: permit2 for token flows #63

merged 19 commits into from
Jul 20, 2024

Conversation

anna-carroll
Copy link
Contributor

@anna-carroll anna-carroll commented Jul 12, 2024

implement permit2 signature-based token transfers for all token flows

@anna-carroll anna-carroll changed the title demo: add permit2 flow feat: permit2 for token flows Jul 17, 2024
@anna-carroll
Copy link
Contributor Author

anna-carroll commented Jul 17, 2024

TODO:

  • import permit2 as a submodule
  • fix EIP-712 encoding for atomic types
  • add tests

@anna-carroll anna-carroll marked this pull request as ready for review July 17, 2024 19:28
src/UsesPermit2.sol Outdated Show resolved Hide resolved
@@ -118,8 +128,7 @@ abstract contract OrderOrigin {
/// @param token - The token to transfer.
/// @custom:emits Sweep
/// @custom:reverts OnlyBuilder if called by non-block builder
function sweep(address recipient, address token) public {
if (msg.sender != block.coinbase) revert OnlyBuilder();
function sweep(address recipient, address token) external {
Copy link
Contributor

Choose a reason for hiding this comment

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

did we make a decision on specifying a min amount swept here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it isn't strictly necessary because sweep should be submitted in a bundle with the corresponding initiate calls; if another Filler gets their bundle submitted first, the second bundle's initiate calls will be rejected (because of the tx nonce) so the bundle should be rejected for that reason.

Copy link
Contributor

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

tests pass, code is clean, architecture makes sense

uint256 num = outputs.length;
bytes32[] memory hashes = new bytes32[](num);
for (uint256 i = 0; i < num; ++i) {
hashes[i] = keccak256(abi.encode(_OUTPUT_TYPEHASH, outputs[i]));
Copy link
Contributor

Choose a reason for hiding this comment

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

so quick note, if the output ever includes another struct or a dynamic type, we can't use abi.encode here. This may be worth documenting here and near the output structdef

returns (Witness memory _witness)
{
_witness.witnessHash =
keccak256(abi.encode(_ENTER_WITNESS_TYPEHASH, EnterWitness(rollupChainId, rollupRecipient)));
Copy link
Contributor

Choose a reason for hiding this comment

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

same note. if the struct ever includes dynamic types we can't use abi.encode

@anna-carroll anna-carroll merged commit 3cbe2fe into main Jul 20, 2024
1 check passed
@anna-carroll anna-carroll deleted the anna/permit2 branch July 20, 2024 19: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