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 #600

Open
danielattilasimon opened this issue Jun 10, 2021 · 2 comments
Open

Improve recovery from outdated hints in SortedTroves #600

danielattilasimon opened this issue Jun 10, 2021 · 2 comments
Labels

Comments

@danielattilasimon
Copy link
Collaborator

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.

@danielattilasimon danielattilasimon added this to the v2 milestone Jun 10, 2021
@danielattilasimon
Copy link
Collaborator Author

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

danielattilasimon added a commit that referenced this issue Jun 21, 2021
… SortedTroves

When 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))`. If we blindly pass these as hints to
`openTrove()` but the transaction ends up pending for so long that the backend
needs 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.

Work around this by replacing the zero address with the other hint.

Workaround for #600.
danielattilasimon added a commit that referenced this issue Jun 21, 2021
… SortedTroves

When 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))`. If we blindly pass these as hints to
`openTrove()` but the transaction ends up pending for so long that the backend
needs 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.

Work around this by replacing the zero address with the other hint.

Workaround for #600.
@RickGriff
Copy link
Collaborator

RickGriff commented Jul 23, 2021

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.

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

No branches or pull requests

3 participants