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: Ekubo Integration #126

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

Conversation

die-herdplatte
Copy link

@die-herdplatte die-herdplatte commented Mar 20, 2025

Copy link
Author

@die-herdplatte die-herdplatte left a comment

Choose a reason for hiding this comment

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

Should Ekubo's callbacks be directly integrated into TychoRouter.sol?

@die-herdplatte die-herdplatte changed the title Ekubo Integration feat: Ekubo Integration Mar 20, 2025
@Haikane Haikane requested a review from tamaralipows March 21, 2025 09:49
Copy link
Contributor

@tamaralipows tamaralipows left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this PR!

Regarding this:

Should Ekubo's callbacks be directly integrated into TychoRouter.sol?

This is discouraged if you can make the callback work with the handleCallback method in the executor - which seems to be the case here. No changes to the main TychoRouter necessary.

Could you please update your branch with the main branch - we have recently pushed a fix that should make CI run properly in external contribution PRs now.

Other than this - I would recommend to add an integration test on for the executor contract which uses data encoded by the swap encoder. Would it be possible for you to add one for the EkuboExecutor? See simple examples here:

testSwapIntegration in BalancerV2ExecutorTest
testMultipleSwapIntegration in UniswapV4ExecutorTest

@die-herdplatte
Copy link
Author

This is discouraged if you can make the callback work with the handleCallback method in the executor - which seems to be the case here. No changes to the main TychoRouter necessary.

Slightly related: Do I need the direct callback implementations on the executor itself (locked and payCallback).
I just implemented them because other executors did the same and it simplifies tests but shouldn't the executor only be callable by the router?

Could you please update your branch with the main branch - we have recently pushed a fix that should make CI run properly in external contribution PRs now.

Done, but seems like there are still issues.

@tamaralipows
Copy link
Contributor

tamaralipows commented Mar 27, 2025

You're right to point out that the executor will only be callable through delegatecalls by the router, and thus those direct callbacks don't need to be implemented. We had included them just for clarity for the user and ease of testing - it's fine if you leave them here though.

One thing that I'd like to double check - does this work through the router? We need to make sure the callback data starts with the executor address in that case:

    function _handleCallback(bytes calldata data) internal {
        address executor = address(uint160(bytes20(data[data.length - 20:])));

I'll take over this PR to double check this - thanks!

About the CI - this should be fixed soon, we are working on this internally.

@die-herdplatte
Copy link
Author

die-herdplatte commented Mar 28, 2025

I just noticed that the executor doesn't use permits to pay Ekubo Core.
I can rectify this until tomorrow if you want.

@tamaralipows
Copy link
Contributor

I just noticed that the executor doesn't use permits to pay Ekubo Core.
I can rectify this until tomorrow if you want.

Sure - that would be a big help!

@die-herdplatte
Copy link
Author

Nevermind, I see that the router uses the permit to transfer tokens to itself before calling the executor's swap function.
Then everything is fine from my side now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

2 participants