interface icon indicating copy to clipboard operation
interface copied to clipboard

chore: permit modifications

Open defispartan opened this issue 2 years ago • 2 comments

General Changes

  • Move permit to external utility (not dependent on V3 Pool)
  • Always display approval tx fallback if permit is enabled
  • Standardize permitConfig file and it's usage to always use lowercase

Developer Notes

Add any notes here that may be helpful for reviewers.


Author Checklist

Please ensure you, the author, have gone through this checklist to ensure there is an efficient workflow for the reviewers.

  • [ ] The base branch is set to main
  • [ ] The title is using Conventional Commit formatting
  • [ ] The Github issue has been linked to the PR in the Development section
  • [ ] The General Changes section has been filled out
  • [ ] Developer Notes have been added (optional)

If the PR is ready for review:

  • [ ] The PR is in Open state and not in Draft mode
  • [ ] The Ready for Dev Review label has been added

Reviewer Checklist

Please ensure you, as the reviewer(s), have gone through this checklist to ensure that the code changes are ready to ship safely and to help mitigate any downstream issues that may occur.

  • [ ] End-to-end tests are passing without any errors
  • [ ] Code style generally follows existing patterns
  • [ ] Code changes do not significantly increase the application bundle size
  • [ ] If there are new 3rd-party packages, they do not introduce potential security threats
  • [ ] If there are new environment variables being added, they have been added to the .env.example file as well as the pertinant .github/actions/* files
  • [ ] There are no CI changes, or they have been approved by the DevOps and Engineering team(s)
  • [ ] Code changes have been quality checked in the ephemeral URL
  • [ ] QA verification has been completed
  • [ ] There are two or more approvals from the core team
  • [ ] Squash and merge has been checked

defispartan avatar Nov 18 '22 06:11 defispartan

  • Ipfs hash: bafybeiaaun7lwteogu5mjffmulr5epvuqzin3i3m454x3ulhr2srmfh2a4
  • Ipfs preview link: https://bafybeiaaun7lwteogu5mjffmulr5epvuqzin3i3m454x3ulhr2srmfh2a4.ipfs.cf-ipfs.com/

github-actions[bot] avatar Nov 18 '22 21:11 github-actions[bot]

📦 Next.js Bundle Analysis

This analysis was generated by the next.js bundle analysis action 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 472.35 KB (🟡 +936 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Two Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load
/governance 62.38 KB (-3 B) 534.73 KB
/staking 20.52 KB (🟡 +26 B) 492.87 KB
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

github-actions[bot] avatar Nov 18 '22 21:11 github-actions[bot]

  • Ipfs hash: bafybeif5whycqfor55c34bl4zmqret6mbajjorlox36e53rlr63prhhr34
  • Ipfs preview link: https://bafybeif5whycqfor55c34bl4zmqret6mbajjorlox36e53rlr63prhhr34.ipfs.cf-ipfs.com/

github-actions[bot] avatar Nov 22 '22 04:11 github-actions[bot]

I generally think this is an improvement for wallets not supporting permit. That said i think the implementation/ux of that is pretty weird.

The path for "doing approval" instead of permit is a link ... which acts like a button? Which at least for me is kinda unexpected and very non-obvious.

Why isn't there just a switch or sth to switch between the two methods? image

Wdyt?

sakulstra avatar Nov 23 '22 11:11 sakulstra

LGTM Screen Shot 2022-11-23 at 11 45 24

bojanaave avatar Nov 23 '22 16:11 bojanaave

@sakulstra My 5 cents on that:

  • it's not a button not to confuse users with 3 buttons (in case if we just put an approve button on top). My guess here, most of users are giving permissions with signed messages just fine. So it's a help link on the bottom to reference when there is a problem.
  • it's not a switch, partially because a button is more preferable imho, and because it requires some space and more UI changes. Also it's 2 clicks versus 1 click. Thus, to make a quick fix it's a link for now. I'm in favour of making a bigger change here in terms of UI, more clear messages and different states, but I also see people struggling here and now.

iamanastasia avatar Nov 23 '22 16:11 iamanastasia

I would add a question to 'Approval doesn't work?' and redirect users to this article instead to learn more details (Learn more link in the tooltip)

iamanastasia avatar Nov 23 '22 16:11 iamanastasia

  • Ipfs hash: bafybeid3cdcqm2cc5rtahwk2rb2elypiebbfxtth3736ffyvynag4ooopu
  • Ipfs preview link: https://bafybeid3cdcqm2cc5rtahwk2rb2elypiebbfxtth3736ffyvynag4ooopu.ipfs.cf-ipfs.com/

github-actions[bot] avatar Nov 23 '22 18:11 github-actions[bot]

@iamanastasia i see your point, but still kinda disagree :)

The thing is that "approval" vs "permit" in most cases is not really a user choice but in the end a wallet choice (it might also be a user choice actually).

If I'm using a wallet not supporting permit, I have to:

  • realize the app tries to submit via permit and that there was an error... or nothing happening (@defispartan commented they don't consistently throw errors)
  • click a link below the grayed out "supply" button (it's no even visually close to the "non reacting" approve button)

Also the 1 vs 2 clicks argument for me is a bit odd.

  • For most users it will still be one click.
  • For users using wallets without permit support it should be 1 additional click once. If permit doesn't work once, it will never work - the switch value could be stored across sessions. So it's "one click once" vs "never again clicking the big approval button, but always clicking a link below the action"

sakulstra avatar Nov 24 '22 13:11 sakulstra

@iamanastasia actually one more thing to support my point.

With the current approach (and the suggested one in this pr) - gas estimation will always be false for permits as it will always be for approval flow (as it's not clear which path the user is taking before the fact).

sakulstra avatar Nov 24 '22 14:11 sakulstra

@iamanastasia actually one more thing to support my point.

With the current approach (and the suggested one in this pr) - gas estimation will always be false for permits as it will always be for approval flow (as it's not clear which path the user is taking before the fact).

Gas estimation is only based on the action, not the approval

defispartan avatar Nov 28 '22 20:11 defispartan

  • Ipfs hash: bafybeig5vqe47fwcogwfvyyxbcymcm5pqmpsiiq3nal6sojeu77g4jbs5q
  • Ipfs preview link: https://bafybeig5vqe47fwcogwfvyyxbcymcm5pqmpsiiq3nal6sojeu77g4jbs5q.ipfs.cf-ipfs.com/

github-actions[bot] avatar Nov 28 '22 21:11 github-actions[bot]

Gas estimation is only based on the action, not the approval

I know, that's what i pointed out via dm some time ago. That is why it's always wrong.

On approval flow, you omit the approval cost which usually is a substantial part of the txn cost (iirc, aave ui includes it because of that). On permit flow the gas estimation displayed is for e.g. supply not supplyWithPermit(which usually is a bit more gas intensive as it includes the permit logic)

sakulstra avatar Nov 28 '22 21:11 sakulstra

Gas estimation is only based on the action, not the approval

I know, that's what i pointed out via dm some time ago. That is why it's always wrong.

On approval flow, you omit the approval cost which usually is a substantial part of the txn cost (iirc, aave ui includes it because of that). On permit flow the gas estimation displayed is for e.g. supply not supplyWithPermit(which usually is a bit more gas intensive as it includes the permit logic)

Added the approval tx to gas estimation in the last commit. The issue with doing estimations for supplyForPermit is that the estimation will fail unless you provide a valid signature.

It would be possible to hide the estimation until the user signs or switch the estimation from supply -> supplyWithPermit after they sign, but this retriggers the tx building useEffect and causes loading/stuttering in the modal which imo is worse UX than just substituting in the supply gas cost (omitting the approval) since these estimations were within the margin of error of gas price movements from my testing.

defispartan avatar Nov 28 '22 23:11 defispartan

  • Ipfs hash: bafybeichp2v4kmsvth7e2z6ijuw6nvllxefx274nuv2kh63h7gqfism4xq
  • Ipfs preview link: https://bafybeichp2v4kmsvth7e2z6ijuw6nvllxefx274nuv2kh63h7gqfism4xq.ipfs.cf-ipfs.com/

github-actions[bot] avatar Nov 29 '22 00:11 github-actions[bot]

@sakulstra Do you think this switcher might be better? Screenshot 2022-11-29 at 13 16 24

Can it be a config button with a dropdown? Screenshot 2022-11-29 at 13 29 38

iamanastasia avatar Nov 29 '22 09:11 iamanastasia

@defispartan

Added the approval tx to gas estimation in the last commit. The issue with doing estimations for supplyForPermit is that the estimation will fail unless you provide a valid signature.

It's exactly the same for supply, gas estimation will fail when there's no approval. Check aave-utilities, there's default gasLimits for e.g. supply to have a estimation in these cases. Probably the one for supplyWithPermit is missing.

@iamanastasia I think both designs are good (personally i'd prefer the toggle, as it's just two options, so hiding behind a settings wheel feels a bit overkill). Not sure sure about the text, but also don't have a better suggestion. Perhaps the button could say "Sign DAI approval" when permit is enabled and "Approve DAI" if not ? - not sure if that makes it clearer.

sakulstra avatar Nov 29 '22 10:11 sakulstra

I'm wondering if the toggle switch will be confusing. Would the user expect some immediate action on toggle? It might not be very clear at first that the toggle will change the function of the 'approve' button.

grothem avatar Nov 29 '22 14:11 grothem

@sakulstra I like the idea that we can store user's choice, but still think that need to play around with designs and wording.

iamanastasia avatar Nov 29 '22 15:11 iamanastasia

This looks to me as a better version than hyperlink at the bottom of the screen. Screenshot 2022-11-30 at 17 39 23

I would recommend to either:

  1. Publish current version and create new task to improve.
  2. OR Make changes to UI as shown on the screenshot above, and return to that issue later.

@sakulstra @defispartan @grothem Thoughts? suggestions?

iamanastasia avatar Nov 30 '22 13:11 iamanastasia

  • Ipfs hash: bafybeif6qtvwitrze2f2zhcxy5s4i2gvmxcn3oolbicui52ckpxp7dm5uq
  • Ipfs preview link: https://bafybeif6qtvwitrze2f2zhcxy5s4i2gvmxcn3oolbicui52ckpxp7dm5uq.ipfs.cf-ipfs.com/

github-actions[bot] avatar Nov 30 '22 15:11 github-actions[bot]

I think this looks really good! One question on the tooltip though, will it be confusing to have that inside the button? Would having it next to the fly out menu link be a better spot?

grothem avatar Nov 30 '22 18:11 grothem

I like it as well, and this new approach will allow us to always generate accurate gas estimations for whichever path the user selects so I think it's worthwhile to do this refactor before shipping

defispartan avatar Dec 01 '22 05:12 defispartan

@grothem yeah, good point. I used uniswap as reference for that. And since we don't have much space for 'Why do I need to approve(?)', I would put it in the button, it's hard to find in dropdown menu.

iamanastasia avatar Dec 01 '22 09:12 iamanastasia

@iamanastasia not so sure about the wording (don't have a better suggestion though). Besides that i think it's good.

The looks a bit weird to me that tx gas cost is no longer attached to the txn buttons. Wouldn't it both fit into the same line?, might be just my personal taste.

sakulstra avatar Dec 01 '22 13:12 sakulstra

@iamanastasia not so sure about the wording (don't have a better suggestion though). Besides that i think it's good.

The looks a bit weird to me that tx gas cost is no longer attached to the txn buttons. Wouldn't it both fit into the same line?, might be just my personal taste.

There is another branch (repay with collateral). And there is Slippage tolerance near gas price. Screenshot 2022-12-08 at 18 35 31

iamanastasia avatar Dec 08 '22 14:12 iamanastasia

  • Ipfs hash: bafybeicm6tl6pqhivwm2nrkgsjyl7pi5q2byuzr2lnha7mvgca56jgmeqi
  • Ipfs preview link: https://bafybeicm6tl6pqhivwm2nrkgsjyl7pi5q2byuzr2lnha7mvgca56jgmeqi.ipfs.cf-ipfs.com/

github-actions[bot] avatar Dec 14 '22 05:12 github-actions[bot]

  • Ipfs hash: bafybeiazbyw32s3fxtcsayiqrh3xqynpsy7nqzx6ot2hjr3am4tfjsnwka
  • Ipfs preview link: https://bafybeiazbyw32s3fxtcsayiqrh3xqynpsy7nqzx6ot2hjr3am4tfjsnwka.ipfs.cf-ipfs.com/

github-actions[bot] avatar Dec 14 '22 05:12 github-actions[bot]

great job! @defispartan

iamanastasia avatar Dec 14 '22 11:12 iamanastasia

LGTM Screen Shot 2022-12-14 at 12 12 35 Screen Shot 2022-12-14 at 12 11 32 Screen Shot 2022-12-14 at 12 11 16

bojanaave avatar Dec 14 '22 14:12 bojanaave