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

STL router #347

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

STL router #347

wants to merge 4 commits into from

Conversation

z0r0z
Copy link
Contributor

@z0r0z z0r0z commented Apr 25, 2022

integrate solmate STL, and reduce mloads

integrate solmate STL, and reduce mloads
wrap erc20 for stl
unchecked {
return i + 1;
return i++;
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't work as expected since it returns the same i value instead of incrementing it. i + 1 is fine and already optimal. I would also remove the comment since it doesn't apply for this generic function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I suppose ++i would work here, but will revert to move this along.

revert increment flow
remove unchecked comment
// Pay the first pool directly.
bento.transfer(params.tokenIn, msg.sender, params.path[0].pool, params.amountIn);
// Call every pool in the path.
// Pool `N` should transfer its output tokens to pool `N+1` directly.
// The last pool should transfer its output tokens to the user.
// If the user wants to unwrap `wETH`, the final destination should be this contract and
// a batch call should be made to `unwrapWETH`.
uint256 n = params.path.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this not an optimisation? I didn't run the numbers but afaik it is (caching-the-length-in-for-loops)

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gasperbr I just did some testing and including cache increases gas cost
image

gist, https://gist.github.com/z0r0z/4187d4f39b9a2a6e89941f9f8e86682b

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me know if this makes sense. I see explanation of how optimizer might be working to cache outside of loop here, https://gist.github.com/hrkrshnn/a1165fc31cbbf1fae9f271c73830fdda

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