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

feat: approval type addition #456

Draft
wants to merge 3 commits into
base: chore/separate-permit
Choose a base branch
from

Conversation

sakulstra
Copy link
Collaborator

@sakulstra sakulstra commented Nov 19, 2022

Probably testing setup should be cleaned up a bit.

The core motivation for this chance is that the current data flow of the app:
a) is predictable
b) extremely wasteful

So the changes proposed here make it slightly less wasteful: hear me out

So when app.aave.com loads it:

  1. fetches data from uiPoolDataProvider including name, symbol, decimals, amount(in wei) for each asset

  2. this data is then formatted so amount is in "units" (so basically shift by decimals)

  3. this amount is then passed to e.g. supply which will then

    1. fetch the decimals (async call, for sth you already know - see 1)
    2. transform to wei
    3. check if isApproved
      1. fetched the decimals (async call, 1, 3.1 - so a third time)
      2. fetch allowance
    4. populates & returns transactions for approval & supply
  4. in the case of permit - throw things away 😅 & start over with

  5. fetching decimals again, because why not https://github.com/aave/aave-utilities/pull/455/files#diff-94e2771eec1338274fb2a401a493a9b6822e75f0ffba272f3e3f921e156ec943R75 - it's 3 seperate calls, fetching name, symbol & decimals. Everything is known already. 4th time fetching decimals for a single txn.

  6. check if in fact it is approved (yep we already did in 3.3, but why not https://github.com/aave/aave-utilities/pull/455/files#diff-94e2771eec1338274fb2a401a493a9b6822e75f0ffba272f3e3f921e156ec943R82)

    1. you probably already guessed it, 5th time fetching decimals

fin

This pr removes, 5,6,7,7.1 by assuming:

  1. you only want to do a permit when the app requests an approval, so you don't have to recheck if it's already approved
  2. expecting you to already know the things you already know

It does not solve the other problems.

Disclaimer: didn't adjust the tests properly yet for latest changes.
Might make sense to abstract some things there.


My recommendation(not in this pr) would be to have a close look at this behavior (in the context of all transactions) and refactor this in depth as it really doesn't make sense.

I think in all cases of:

const approved: boolean = await isApproved({
      token: reserve,
      user,
      spender: this.poolAddress,
      amount,
});

You pass amount as units, just to fetch decimals , which you fetched 2 lines above in essentially all cases.
Probably a first step would be to just directly pass wei here. 1 async call less.

The next step might be:
a) altering the signature to just accept decimals (as you already know them)
b) expect to already get the wei (not sure how good this fits into the application data flow)

@sakulstra sakulstra marked this pull request as draft November 19, 2022 23:39
@sakulstra sakulstra mentioned this pull request Nov 20, 2022
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.

1 participant