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

[Viewer] Only update order balance if it isn't filtered out #670

Closed
wants to merge 1 commit into from

Conversation

nlordell
Copy link
Contributor

This PR updates an order balance only if it is considered valid (i.e. matches the token filter or is valid for the specified batch) for some super awesome gas savings.

Test Plan

Deployed on Rinkeby and was able to go through 7 pages of unfiltered orders compared to 3 pages of unfiltered orders. We identify calls to getEncodedUserOrders by calls to getUsersPaginated since it is only used there.

@nlordell nlordell requested a review from a team April 17, 2020 16:08
@nlordell
Copy link
Contributor Author

This might cause problems though, since retrieving "full" pages (i.e. pages with many orders that are considered valid) is considerably more expensive than "empty" pages (i.e. pages with few orders that are considered valid) and hence, the gas check that we currently perform might not be good enough.

Copy link
Contributor

@bh2smith bh2smith left a comment

Choose a reason for hiding this comment

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

Short and sweet, wins the race! Super awesome gas savings you have there.

@fleupold
Copy link
Contributor

This might cause problems though, since retrieving "full" pages (i.e. pages with many orders that are considered valid) is considerably more expensive than "empty" pages (i.e. pages with few orders that are considered valid) and hence, the gas check that we currently perform might not be good enough.

Yes, I think this is quite worrying as it makes our already brittle logic there even harder to reason about. I would like to see us move to a fixed "worst-case gas required for next loop" logic (maybe with a factor for page size), before landing this change.

I do agree the gas savings are likely going to be awesome.

@nlordell
Copy link
Contributor Author

"worst-case gas required for next loop"

Time to tenderly again! Woohoo!

before landing this change.

I also agree, I'll leave the PR open for now until we implement something like that.

@nlordell
Copy link
Contributor Author

Created #678 and placed this PR on hold.

@nlordell nlordell closed this Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants