Skip to content

feat: implementing permit2 with pay with any token#5926

Merged
JackShort merged 18 commits into
mainfrom
jack/payWithAnyTokenPermit2
Feb 9, 2023
Merged

feat: implementing permit2 with pay with any token#5926
JackShort merged 18 commits into
mainfrom
jack/payWithAnyTokenPermit2

Conversation

@JackShort
Copy link
Copy Markdown
Contributor

@JackShort JackShort commented Feb 4, 2023

  • Adds permit 2 approval flow to the pay with any token on nft side

Screenshot 2023-02-05 at 10 55 22 PM

Screenshot 2023-02-05 at 10 58 31 PM

Screenshot 2023-02-05 at 10 59 01 PM

@vercel
Copy link
Copy Markdown

vercel Bot commented Feb 4, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
interface ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 9, 2023 at 5:35PM (UTC)

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Feb 4, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: b4beec6
Status:⚡️  Build in progress...

View logs

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 4, 2023

Codecov Report

Base: 12.86% // Head: 12.89% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (339d08b) compared to base (89e438b).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5926      +/-   ##
==========================================
+ Coverage   12.86%   12.89%   +0.02%     
==========================================
  Files         408      408              
  Lines       12815    12816       +1     
  Branches     4491     4492       +1     
==========================================
+ Hits         1649     1652       +3     
+ Misses      11159    11157       -2     
  Partials        7        7              
Impacted Files Coverage Δ
src/components/Button/index.tsx 26.78% <0.00%> (-0.17%) ⬇️
src/pages/Vote/Landing.tsx 0.00% <0.00%> (ø)
src/pages/Pool/PositionPage.tsx 0.00% <0.00%> (ø)
...c/components/FeatureFlagModal/FeatureFlagModal.tsx 0.00% <0.00%> (ø)
src/hooks/usePermitAllowance.ts 6.45% <0.00%> (+6.45%) ⬆️
src/components/Tokens/loading.tsx 25.00% <0.00%> (+25.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment thread cypress/e2e/nfts.test.ts Outdated
cy.get(getTestSelector('nft-details-description-text')).should('not.exist')
cy.get(getTestSelector('nft-details-toggle-bag')).eq(1).click()
cy.get(getTestSelector('nft-bag')).should('exist')
// TODO: Enable when universal router is deployed to goerli these tests fail because it is not deployed there
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you go into more detail about this failure? Could we instead make the bag or test conditional based off chain instead of removing it entirely?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

copied what web did and changed the hook to not be enabled on chains where universal router is not deployed an added an enabled flag

Comment thread src/nft/components/bag/BagFooter.tsx Outdated
Comment thread src/nft/components/bag/BagFooter.tsx Outdated
display: flex;
justify-content: center;
margin: 12px 0 !important;
margin-bottom: 10px !important;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I don't think the !important is needed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

surprisingly it is

Comment thread src/nft/components/bag/BagFooter.tsx Outdated
buttonText = <Trans>Connect wallet</Trans>
} else if (usingPayWithAnyToken && tradeState !== TradeState.VALID) {
disabled = true
buttonText = <Trans>Fetching Route</Trans>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While fetching route I can still enter the purchasing state and fire off multiple txs
multipleTxs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is actually a bug on main .... fixed

Comment thread src/nft/hooks/usePermit2Approval.ts Outdated
import { useCallback, useMemo, useState } from 'react'
import invariant from 'tiny-invariant'

export default function usePermit2Approval(amount?: CurrencyAmount<Token>, maximumAmount?: CurrencyAmount<Token>) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does web not already have a hook for approving the use of Permit2?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

they don't this is the code from web reduced to a hook

@JackShort JackShort requested a review from cbachmeier February 8, 2023 00:07
Copy link
Copy Markdown
Contributor

@cbachmeier cbachmeier left a comment

Choose a reason for hiding this comment

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

Small bug. Other than that works well!


const PayButton = styled(Row)<{ disabled?: boolean }>`
background: ${({ theme }) => theme.accentAction};
const PayButton = styled.button<{ $backgroundColor: string }>`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing gap between spinner and text. Content is also no longer centered.
Screen Shot 2023-02-08 at 15 13 09

@JackShort JackShort merged commit 3eaeb65 into main Feb 9, 2023
@JackShort JackShort deleted the jack/payWithAnyTokenPermit2 branch February 9, 2023 18:41
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.

3 participants