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.

Sign up for GitHub

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

Jump to bottom

fix: wc params for argent mobile #118

Merged
merged 4 commits into from
Aug 6, 2024
Merged

fix: wc params for argent mobile #118

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
acb9a5c
fix: wc params for argent mobile
bluecco Jul 29, 2024
792deaf
fix: udpate argent mobile login link
bluecco Aug 6, 2024
d1d11dc
chore: revert prod argent mobile login
bluecco Aug 6, 2024
4e4a988
chore: remove comments
bluecco Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 3 additions & 14 deletions src/connectors/argentMobile/modal/argentModal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,21 +72,10 @@ class ArgentModal {
const wcParam = encodeURIComponent(wcUri)
const href = encodeURIComponent(window.location.href)

/*
// KEEPING IN CASE NEEDS TO BE REVERTED
const desktopWcParam = encodeURIComponent(
`${this.mobileUrl}app/wc?uri=${wcParam}`,

this.showModal({
desktop: `${this.bridgeUrl}?wc=${desktopWcParam}&device=desktop`,
ios: `${this.mobileUrl}app/wc?uri=${wcParam}&href=${href}&device=mobile`,
android: `${this.mobileUrl}app/wc?uri=${wcParam}&href=${href}&device=mobile`,
})

) */
const desktopWcParam = encodeURIComponent(wcParam)
Copy link

Choose a reason for hiding this comment

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

I cannot properly review it without the argent login changes, but that doesn't look correct. it's basically encodeURIComponent(encodeURIComponent(wcUri)) which doesn't make much sense.

I the commented out code it was necessary because desktopWcParam was an actual link

Copy link
Contributor Author

@bluecco bluecco Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@kamilargent ok, I probably misunderstood last time then

so, in this case, the following should be enough:

const wcParam = encodeURIComponent(wcUri)

this.showModal({
      desktop: `${this.bridgeUrl}?wc=${wcParam}&href=${href}&device=desktop`,
      ios: `${this.mobileUrl}app/wc?uri=${wcParam}&href=${href}&device=mobile`,
      android: `${this.mobileUrl}app/wc?uri=${wcParam}&href=${href}&device=mobile`,
    })

or we need to revert the commented code

 // KEEPING IN CASE NEEDS TO BE REVERTED
    const desktopWcParam = encodeURIComponent(
      `${this.mobileUrl}app/wc?uri=${wcParam}`,
    this.showModal({
      desktop: `${this.bridgeUrl}?wc=${desktopWcParam}&device=desktop`,
      ios: `${this.mobileUrl}app/wc?uri=${wcParam}&href=${href}&device=mobile`,
      android: `${this.mobileUrl}app/wc?uri=${wcParam}&href=${href}&device=mobile`,
    })

Copy link

Choose a reason for hiding this comment

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

Everything depends on how you treat the wc param on the argent login side, when creating the QR code.

  1. If it's just getting encoded into the QR code, then, the previous code should be used (although new config.mobileUrl will have to be used as we need here https;//www.argent.xyz/ link instead of argent:// for desktop flow)
  2. If it's just getting the wc param end creates a new qr link, then we are ok with just const wcParam = encodeURIComponent(wcUri)
const wcParam = getQueryParam(link, "wc")
const href = getQueryParam(link, "href")

const qrCode = "https://www.argent.xyz/app/wc?uri=${wcParam}&href=${href}&device=desktop"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok @kamilargent
I think #2 is ok since the url creation is managed in argent-login

// argent-login code
 <FirstConnection
            qrCodeData={`https://www.argent.xyz/app/wc?uri=${walletConnectUrl}`}
          />

Copy link

Choose a reason for hiding this comment

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

👍 as long as the walletConnectUrl is the encoded version of the wcUri we should be good


this.showModal({
desktop: `${this.bridgeUrl}?wc=${wcParam}&device=desktop`,
desktop: `${this.bridgeUrl}?wc=${desktopWcParam}&href=${href}&device=desktop`,
ios: `${this.mobileUrl}app/wc?uri=${wcParam}&href=${href}&device=mobile`,
android: `${this.mobileUrl}app/wc?uri=${wcParam}&href=${href}&device=mobile`,
})
Expand All @@ -109,7 +98,7 @@ class ArgentModal {
this should be ignored and not considered valid as it's only used for automatically redirecting the users to approve or reject a signing request.
*/
this.showModal({
desktop: `${this.bridgeUrl}?action=sign&device=desktop`,
desktop: `${this.bridgeUrl}?action=sign&device=desktop&href=${href}`,
ios: `${this.mobileUrl}app/wc/request?href=${href}&device=mobile`,
android: `${this.mobileUrl}app/wc/request?href=${href}&device=mobile`,
})
Expand Down