fix: precision factor is useless #335
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
changed the title
fubuloubu
approved these changes
May 11, 2021
There was a problem hiding this comment.
I think there was one or two cases where this extra precision factor did in fact help. Regardless, it would be nice to have an analysis that shows more completely that it is not doing the job it is meant to do.
Little background: this was added in a previous audit to account for tokens with decimals like GUSD, where 1 token = $0.01, so precision loss could quickly add up.
That's also what I tought but I have reviewed all our calculations following the comment from @zgfzgf and it can only be useful if we multiply after a division that we do not have. |
|
do we have a test that was targeting the precision factor logic specifically? if not can we add it to make sure there's no regression here? |
We have tests running with two decimals tokens. |
ghost
merged commit
fubuloubu
added a commit
that referenced
this pull request
May 16, 2021
* docs: withdraw can empty available funds (#302) * fix: prevent revoking non existing strategy (#303) * fix: prevent revoking non existing strategy * refactor: fix typo Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * chore: remove guestList (#308) * fix: strategy migration only through Vault (#309) * fix: migratiion should only be perforned by vault * docs: document new strategy prerequisites * fix: typo Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: make sure _assessFees isn't called twice within the same block (#326) * fix: avoid duplicates on withdrawal queue (#299) * fix: avoid duplicates on withdrawal queue * fix: use assert instead of raise Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: use only one loop * fix: use min approach * refactor: queue should not add or remove entries * feat: do not replace strategy * refactor: clarity Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * docs: comment Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: refactor for clarity Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * docs: token should not be inflationary, deflationary or transfer fees (#331) * docs: loss debt ratio flow (#332) * docs: loss will udate debt ratio, debt is adjusted on next report * docs: fixup comment Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: prevent fees to be above 100% (#328) Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: precision factor is useless (#335) * fix: precision factor is useless most of the time * fix: remove entirely precision factor Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: prevent division per zero (#337) * fix: prevent division per zero Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com> * fix: do not relly on user manipulable values * fix: strategy emergency mode * fix: symplify math Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com> Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com>
orbxball
pushed a commit
to orbxball/yearn-vaults
that referenced
this pull request
Aug 8, 2021
* docs: withdraw can empty available funds (yearn#302) * fix: prevent revoking non existing strategy (yearn#303) * fix: prevent revoking non existing strategy * refactor: fix typo Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * chore: remove guestList (yearn#308) * fix: strategy migration only through Vault (yearn#309) * fix: migratiion should only be perforned by vault * docs: document new strategy prerequisites * fix: typo Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: make sure _assessFees isn't called twice within the same block (yearn#326) * fix: avoid duplicates on withdrawal queue (yearn#299) * fix: avoid duplicates on withdrawal queue * fix: use assert instead of raise Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: use only one loop * fix: use min approach * refactor: queue should not add or remove entries * feat: do not replace strategy * refactor: clarity Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * docs: comment Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: refactor for clarity Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * docs: token should not be inflationary, deflationary or transfer fees (yearn#331) * docs: loss debt ratio flow (yearn#332) * docs: loss will udate debt ratio, debt is adjusted on next report * docs: fixup comment Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: prevent fees to be above 100% (yearn#328) Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: precision factor is useless (yearn#335) * fix: precision factor is useless most of the time * fix: remove entirely precision factor Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: prevent division per zero (yearn#337) * fix: prevent division per zero Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com> * fix: do not relly on user manipulable values * fix: strategy emergency mode * fix: symplify math Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com> Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com>
sambacha
pushed a commit
to sambacha/yearn-vaults
that referenced
this pull request
Sep 7, 2021
* docs: withdraw can empty available funds (yearn#302) * fix: prevent revoking non existing strategy (yearn#303) * fix: prevent revoking non existing strategy * refactor: fix typo Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * chore: remove guestList (yearn#308) * fix: strategy migration only through Vault (yearn#309) * fix: migratiion should only be perforned by vault * docs: document new strategy prerequisites * fix: typo Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: make sure _assessFees isn't called twice within the same block (yearn#326) * fix: avoid duplicates on withdrawal queue (yearn#299) * fix: avoid duplicates on withdrawal queue * fix: use assert instead of raise Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: use only one loop * fix: use min approach * refactor: queue should not add or remove entries * feat: do not replace strategy * refactor: clarity Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * docs: comment Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: refactor for clarity Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * docs: token should not be inflationary, deflationary or transfer fees (yearn#331) * docs: loss debt ratio flow (yearn#332) * docs: loss will udate debt ratio, debt is adjusted on next report * docs: fixup comment Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: prevent fees to be above 100% (yearn#328) Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: precision factor is useless (yearn#335) * fix: precision factor is useless most of the time * fix: remove entirely precision factor Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> * fix: prevent division per zero (yearn#337) * fix: prevent division per zero Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com> * fix: do not relly on user manipulable values * fix: strategy emergency mode * fix: symplify math Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com> Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> Co-authored-by: fp-crypto <83050944+fp-crypto@users.noreply.github.com>
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As @zgfzgf pointed out on #333 the precision factor isn't improving precision most of the time.
It would only be useful if we were doing a multiplication after a division.