interface
interface copied to clipboard
chore: permit modifications
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 inDraft
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
- Ipfs hash: bafybeiaaun7lwteogu5mjffmulr5epvuqzin3i3m454x3ulhr2srmfh2a4
- Ipfs preview link: https://bafybeiaaun7lwteogu5mjffmulr5epvuqzin3i3m454x3ulhr2srmfh2a4.ipfs.cf-ipfs.com/
📦 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.
- Ipfs hash: bafybeif5whycqfor55c34bl4zmqret6mbajjorlox36e53rlr63prhhr34
- Ipfs preview link: https://bafybeif5whycqfor55c34bl4zmqret6mbajjorlox36e53rlr63prhhr34.ipfs.cf-ipfs.com/
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?
Wdyt?
LGTM
@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.
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)
- Ipfs hash: bafybeid3cdcqm2cc5rtahwk2rb2elypiebbfxtth3736ffyvynag4ooopu
- Ipfs preview link: https://bafybeid3cdcqm2cc5rtahwk2rb2elypiebbfxtth3736ffyvynag4ooopu.ipfs.cf-ipfs.com/
@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"
@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).
@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
- Ipfs hash: bafybeig5vqe47fwcogwfvyyxbcymcm5pqmpsiiq3nal6sojeu77g4jbs5q
- Ipfs preview link: https://bafybeig5vqe47fwcogwfvyyxbcymcm5pqmpsiiq3nal6sojeu77g4jbs5q.ipfs.cf-ipfs.com/
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)
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
notsupplyWithPermit
(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.
- Ipfs hash: bafybeichp2v4kmsvth7e2z6ijuw6nvllxefx274nuv2kh63h7gqfism4xq
- Ipfs preview link: https://bafybeichp2v4kmsvth7e2z6ijuw6nvllxefx274nuv2kh63h7gqfism4xq.ipfs.cf-ipfs.com/
@sakulstra Do you think this switcher might be better?
Can it be a config button with a dropdown?
@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.
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.
@sakulstra I like the idea that we can store user's choice, but still think that need to play around with designs and wording.
This looks to me as a better version than hyperlink at the bottom of the screen.
I would recommend to either:
- Publish current version and create new task to improve.
- OR Make changes to UI as shown on the screenshot above, and return to that issue later.
@sakulstra @defispartan @grothem Thoughts? suggestions?
- Ipfs hash: bafybeif6qtvwitrze2f2zhcxy5s4i2gvmxcn3oolbicui52ckpxp7dm5uq
- Ipfs preview link: https://bafybeif6qtvwitrze2f2zhcxy5s4i2gvmxcn3oolbicui52ckpxp7dm5uq.ipfs.cf-ipfs.com/
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?
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
@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 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.
@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.
- Ipfs hash: bafybeicm6tl6pqhivwm2nrkgsjyl7pi5q2byuzr2lnha7mvgca56jgmeqi
- Ipfs preview link: https://bafybeicm6tl6pqhivwm2nrkgsjyl7pi5q2byuzr2lnha7mvgca56jgmeqi.ipfs.cf-ipfs.com/
- Ipfs hash: bafybeiazbyw32s3fxtcsayiqrh3xqynpsy7nqzx6ot2hjr3am4tfjsnwka
- Ipfs preview link: https://bafybeiazbyw32s3fxtcsayiqrh3xqynpsy7nqzx6ot2hjr3am4tfjsnwka.ipfs.cf-ipfs.com/
great job! @defispartan
LGTM