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

fix: bug in calculation of the next id in MultiTroveGetter #801

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

EmperorOrokuSaki
Copy link

The function skips nodes when using continuation-based pagination.

When calling getDebtPerInterestRateAscending(collIndex, startId, maxIter), the function does:
currId = sortedTroves.getPrev(_startId);

This causes a skip when chaining calls. For example:

- Returns trove with 15% rate, debt: 12005.07
- currId: 46086...0131

Call 2: startId = 46086...0131, maxIter = 1
- Returns trove with 16% rate, debt: 17874.79
- currId: 77004...2374

But when fetching multiple at once:

Call 3: startId = 0, maxIter = 3
[
  [0x0, 15%, 12005.07],
  [0x8cf5..8460, 15.1%, 3546.81],  <-- This node was skipped in pagination
  [0x0, 16%, 17874.79]
]

The 15.1% trove is missing when paginating one by one because we're using the returned currId as the next startId. When we do this, getPrev(startId) skips over the startId node itself.

One fix would be modifying the startId logic in the function:
currId = _startId == 0 ? sortedTroves.getPrev(_startId) : _startId;

This would preserve the node we're starting from rather than skipping to its prev immediately.

The current workaround requires making an extra call to sortedTroves.getNext() between paginated fetches, but that adds some costs and latency that could be avoided with the suggested fix (and the IR canister should try to avoid it as much as possible as the calls take a long time already).

Copy link
Collaborator

@danielattilasimon danielattilasimon left a comment

Choose a reason for hiding this comment

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

Looks good!

@@ -126,7 +126,7 @@ contract MultiTroveGetter is IMultiTroveGetter {
assert(address(sortedTroves) != address(0));

data = new DebtPerInterestRate[](_maxIterations);
currId = sortedTroves.getPrev(_startId);
currId = _startId == 0 ? sortedTroves.getPrev(_startId) : _startId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch! We could also use sortedTroves.getLast() when _startId == 0, which might be more readable.

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