Repository navigation
feat: default wallet picker to in-page modal - #2379
singhyash05 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Hi @singhyash05
Thanks for the PR, the new modal looks very nice!
I have a few high-level things:
- Could you add both "connect" buttons to the example-ping dapp?
- The order of the items listed is different from the popup picker. I believe in the popup we have the order: 1. default remotes (defaultAdapters) 2. additional by dapp (additionalAdapters) 3. announced (installed) 4. suggested (not-installed, if enableSuggestedWallets)
- WalletConnect entry opens a blank tab
- WalletConnect URL/QR is not showing
- CI does not pass
We (@alexmatson-da and I) will provide a more detailed review shortly.
| }, | ||
| "ledgerApi": { | ||
| "baseUrl": "http://127.0.0.1:5003" | ||
| "baseUrl": "http://127.0.0.1:2975" |
There was a problem hiding this comment.
I don't see a reason for these ports to change: these networks are configured to use a Canton node started outside of localnet (pnpm start:canton), and has a default ledger API port of 5003.
There is already a WG network defined for localnet instances in the config, that should be the one selected on the login screen (when using pnpm start:localnet)
| export const WALLET_PICKER_MODAL_HOST = '[data-swk-wallet-picker-modal]' | ||
|
|
||
| export type WalletPickerSurface = { | ||
| kind: 'modal' | 'popup' |
There was a problem hiding this comment.
do we really want to support both modal and popup style pickers? IMO I don't really see a reason to continue to provide the popup as an option, I'd be in favor of going full modal-only
cc @mjuchli-da @joel-da for thoughts
| // Race the connection against a "back" click in the picker | ||
| // so the user can abort a slow attempt and pick again. | ||
| const connectPromise = discovery.connect(targetId) | ||
| connectPromise.catch(() => { | ||
| // Swallow late rejections if the user already went back. | ||
| }) | ||
| const outcome = await Promise.race([ | ||
| connectPromise.then(() => 'connected' as const), | ||
| waitForWalletPickerModalBack().then( | ||
| () => 'back' as const | ||
| ), | ||
| ]) | ||
|
|
||
| if (outcome === 'back') { | ||
| // Abort the in-flight attempt and re-await a selection. | ||
| try { | ||
| await discovery.disconnect() | ||
| } catch { | ||
| // best-effort teardown of any partial session | ||
| } | ||
| this.client = null | ||
|
|
||
| try { | ||
| const retrySelection = | ||
| await this.waitForPickerRetry() | ||
| connectionAttempts.dispatchEvent( | ||
| new CustomEvent<WalletPickerEntry>('attempt', { | ||
| detail: retrySelection, | ||
| }) | ||
| ) | ||
| } catch (retryError) { | ||
| cleanup() | ||
| reject(retryError) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
I wonder if using an AbortController here can help clean up the code and make it more concise/idiomatic. I dont have much first-hand experience with this API, so I leave it to you to see if its worth the gain.
My basic idea/sketch of what it might look like:
const abortController = new AbortController()
// when we create the wallet picker, we give it an optional abortController
export async function pickWalletModal(
entries: WalletPickerEntry[],
abortController?: AbortController
): Promise<WalletPickerResult> {
active?.destroy()
active = new WalletPickerModalController(entries, abortController)
return active.awaitSelection()
}
// inside the wallet picker implementation, we call abort when the user clicks the back button
awaitBack(): Promise<void> {
if (this.backRequested) {
this.backRequested = false
// Invoke the abort signal here
this.abortController.abort()
}
return new Promise<void>((resolve) => {
this.backPending = { resolve }
})
}
// ----------------------
// later, when we call discovery.connect, we pass in the associated *signal* from the same instance (also optional)
discovery.connect(targetId, { signal: abortController.signal })
// inside the implementation, tear down
async connect(providerId?: ProviderId | undefined, signal?: AbortSignal | undefined): Promise<void> {
let targetId = providerId
// ...
signal.onabort = () => {
this.disconnect()
// ... other cleanup
}
}may not be totally accurate, but thats the general idea
|
Took a review over, in addition to Marc's comments I also have a few high-level items after checking out the branch:
|
Ship the production wallet picker as an in-page modal (SDK default) while keeping the popup path via setWalletPicker(pickWallet). Wire dapp-sdk + WalletConnect QR for the modal, add unit/e2e coverage, adapt ping disconnect status + extension helpers, and point WG test ledger URLs at localnet 2975. Signed-off-by: singhyash05 <yashsingh5609@gmail.com>
Restore ledger port 5003, match popup list order, fix WC QR (no blank tab), Firefox install badge, custom WG localStorage, ping dual connect + status clear, and modal e2e popup race. Signed-off-by: singhyash05 <yashsingh5609@gmail.com>
407272a to
c1f9391
Compare
|
Thanks @mjuchli-da @alexmatson-da addressed the review feedback on Marc
Alex
Happy to take another pass once CI reports. |
|
Thanks for taking the time to contribute this PR. After reviewing the changes, we believe the submission falls under the cases described in our AI Policy concerning insufficiently reviewed AI-generated output. In particular, the size and structure of the changes suggest that substantial portions were generated with AI without the level of self-review we require before submitting a PR. Please feel free to reopen once the a self-review was conducted and code quality has been improved. |
4 participants
Why
dApps should open the wallet picker as an in-page modal by default. Popup stays available with
setWalletPicker(pickWallet). Continues #2237.What we did
Took the modal from draft shape into something we could ship as the SDK default, then rewired everything that assumed popup.
pickWalletModal; popup is opt-in; connected/error/retry/back split by modal vs popup; WalletConnect QR mounts in the modalopenWalletPicker, modal host/row locators, WG connect) so e2e drives the new path instead of waiting on a picker popupwalletpicker-modal.spec.tsfor open / backdrop+back / live remote connectwalletpicker.spec.tsfor modal error UX (in-modal alert, not popup "Try Again"). Hit a real lifecycle bug: after modal connect the WG login window closes, so WG popup logout no longer clears dApp Status. Switched that step to dApp Disconnect and fixed pingApp/useStatusso Status clears on disconnectping-page.tswith the modal defaultLedger port (
2975)Localnet exposes JSON Ledger API on
2975, not5003. Updatedwallet-gateway/test/config.jsonandConfig.test.tsso bootstrap/connect e2e hits the real localnet port (same asexample-config).Testing (what we actually ran)
core-wallet-ui-components,dapp-sdk,core-wallet-test-utils,wallet-gateway-remote)pnpm clean:all→build:all --skip-nx-cache→test:allstart:all. First connect runs failed after remotecleanwiped sqlite (emptynetworks). Reseeded DB, confirmed 6 networks, re-ran until green:Result: 4/4 passed.
Test plan
test:allsetWalletPicker(pickWallet)restores popup