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 underlyingBalance computing #114

Closed
wants to merge 4 commits into from
Closed

Fix underlyingBalance computing #114

wants to merge 4 commits into from

Conversation

alexandre-abrioux
Copy link
Contributor

Hi!

First of all, thank you for the dedication your putting on this project, the new protocol is awesome and well documented, it's a pleasure to work with.

I'm using aave-js and more specifically the pool-math helper to approximate users reserves balances from data previously fetched from the blockchain.

I found the library very useful to compute current stableBorrows with the getCompoundedStableBalance method, and current variableBorrows with the getCompoundedBalance method.

However I couldn't verify in my tests that the getCompoundedBalance method could be used to reliably compute the current underlyingBalance. With this in mind I tried to debug the method and to find a fix.

Test case

First of all I added a (failing) test case to try to verify the claim I'm bringing forward. You can checkout to the "add failing test case" commit to run it: cd01845

Fix

After that I basically translated to Javascript the logic found in the protocol:

; to create a new function currentATokenBalance specifically to compute the underlyingBalance.

You can find the new code in the commit "fix compute underlyingBalance": 3e2edb6.

Please give me your opinion on this, I may have overlooked something. Tell me what you think :)

Misc

I had to fix the eslint config to be able to commit because, as you can see here in the CI the command yarn lint is broken.

The issue comes from the fact that prettier/@typescript-eslint is still used in the project while it has been deprecated (see here).

However, I could not make it work by just modifying the .eslintrc.json, I think it is due to this issue: jaredpalmer/tsdx#498 (comment). tsdx doesn't seem to take into account the extends part of the .eslintrc.json config file. I had to migrate the config to the package.json file. It works for me now!

@@ -127,3 +128,56 @@ export function calculateAverageRate(
.multipliedBy(SECONDS_PER_YEAR)
.toString();
}

export function currentATokenBalance(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is essentially an exact implementation of calculateCumulatedBalance from v1, let's ping @kyzia551 and ask what was the rational behind changing it in first place.

It's essentially the question, why the change from:
https://github.com/aave/aave-js/blob/master/src/v1/computations-and-formatting.ts#L207 to
https://github.com/aave/aave-js/blob/master/src/v2/computations-and-formatting.ts#L78

part of the changes seem to be the removal of redirection, but I have problems following the rational behind the other changes

@sakulstra
Copy link
Contributor

sakulstra commented Mar 8, 2021

@alexandre-abrioux thx for your efforts & investigating the issue! Good catch 🔍 👀

Is most of this functionality was already in v1 I created a pr to move it from v1 to common and reuse it in v2 #115.
The eslint changes are quite good as well (seems like these issues slipped in with merging all these eslint updates last week) - would you mind reopening a pr, cherry picking the lint changes?

@alexandre-abrioux
Copy link
Contributor Author

@sakulstra Thanks for your answer! Nice find, I didn't work with v1 so I wasn't aware about that part of the SDK :)
Regarding eslint sure! I'll do this right away.
I'll close this PR then (PR moved to #115).

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.

None yet

2 participants