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

Improve recovery from outdated hints in SortedTroves #15

Closed
bingen opened this issue Mar 3, 2023 · 1 comment
Closed

Improve recovery from outdated hints in SortedTroves #15

bingen opened this issue Mar 3, 2023 · 1 comment

Comments

@bingen
Copy link
Collaborator

bingen commented Mar 3, 2023

See: liquity/dev#600

The current version of SortedTroves could be further improved for the case when hints become outdated because the NICR of the inserted Trove increases over time. This is a fairly common occurance due to the decay of the borrowing fee.

Let’s say we're trying to create a new Trove that has a lower ICR than the current lowest ICR (i.e. inserting into the tail of the list). findInsertPosition() returns (addressOfLowestICRTrove, address(0)) in this case. If we blindly pass these as hints to openTrove() (as we currently do in the middleware), but the transaction ends up pending for so long that we need to look for a new insert position, we start the traversal in the wrong direction (from the wrong end of the list), and end up having to traverse over almost the entire list.

Workaround for the current version is to pass (addressOfLowestICRTrove, addressOfLowestICRTrove) as hints.

On 2021/7/23:
We just had a user who had the lowest ICR trove in the system, trying to adjust his trove to go lower. The tx failed before broadcast - presumably because gas estimation didn't allow it.

I believe this is the edge case DeFiSaver found - and I mistakenly thought the solution outlined above also covered it. The problem is that for the last trove in the list, the addressOfLowestICRTrove is their own address - and the reinsertion first removes it before the insert. The system can't find it in the list, so starts at the opposite end, and runs out of gas.

The DefiSaver fix was to check for this specific edge case: if the trove is last in the list, use the 2nd last trove's address as both hints.

@bingen
Copy link
Collaborator Author

bingen commented Jul 29, 2024

Done in #94

@bingen bingen closed this as completed Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant