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

[LILA-7980] Do not throw on zero fee returned #59

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

0xmichalis
Copy link
Member

There can be edge cases in the pool where the pool may provide one or more wei in the calculation
functions and the calculation functions return zero.

For example, this can happen when CHAR is using a
new feature called minimumTCLSeedingThreshold that:

  1. avoids charging fees to deposits that keep the total underlying supply below a certain threshold
  2. charges fees partially when the deposit brings the total underlying supply above the threshold
  3. charges fees normally above the threshold

Given a deposit that falls under 2, it may be that the chargeable amount is one or more few wei. In this scenario, the pool currently has to handle an error returned by the fee calculator which decreases devexp and complicates pool code.

On the other hand, the pool code is already written to loop through the returned arrays that are part of the FeeDistribution struct so returning a struct with empty arrays means this edge case can be handled in the pool seamlessly.

There can be edge cases in the pool where the pool
may provide one or more wei in the calculation
functions and the calculation functions return zero.

For example, this can happen when CHAR is using a
new feature called `minimumTCLSeedingThreshold` that:
1. avoids charging fees to deposits that keep the total
   underlying supply below a certain threshold
2. charges fees partially when the deposit brings the
   total underlying supply above the threshold
3. charges fees normally above the threshold

Given a deposit that falls under 2, it may be that
the chargeable amount is one or more few wei. In this
scenario, the pool currently has to handle an error
returned by the fee calculator which decreases devexp
and complicates pool code.

On the other hand, the pool code is already written
to loop through the returned arrays that are part of
the FeeDistribution struct so returning a struct with
empty arrays means this edge case can be handled in
the pool seamlessly.
@0xmichalis 0xmichalis requested a review from aspiers October 29, 2024 16:37
@0xmichalis 0xmichalis marked this pull request as ready for review October 29, 2024 16:37
Copy link
Member

@aspiers aspiers left a comment

Choose a reason for hiding this comment

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

LGTM but I don't get why the existing code throws when a zero fee is returned?

@0xmichalis
Copy link
Member Author

0xmichalis commented Oct 29, 2024

LGTM but I don't get why the existing code throws when a zero fee is returned?

Maybe @kosecki123 @PawelTroka can help answer since they authored it? My guess is that there was no use of an empty response before, whereas failing was easier to catch and identify these cases where fee can be zero.

@0xmichalis 0xmichalis merged commit f9888eb into ToucanProtocol:main Oct 29, 2024
1 check passed
@0xmichalis 0xmichalis deleted the do-not-throw-on-zero-fee branch October 29, 2024 22:30
@aspiers
Copy link
Member

aspiers commented Oct 30, 2024

No, I mean I couldn't see where it throws in this case. Perhaps I should have been looking outside the code affected by the PR?

@0xmichalis
Copy link
Member Author

0xmichalis commented Oct 30, 2024

No, I mean I couldn't see where it throws in this case. Perhaps I should have been looking outside the code affected by the PR?

It used to throw in L209. This can happen when the caller provides a very small amount that can cause the fee calculation to result in zero. A few test cases were changed that exemplified the revert, eg. this one used 1 wei as a deposit amount which cannot be divided further. All that being said, I think we probably need to revert, see Slack for more info.

See also this PR (this CI run specifically with a new integration test added to fail because of this exact edge case).

@0xmichalis
Copy link
Member Author

Reverting in #60

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