Skip to content

Notifications: Fix - open the token wallet or its account - #2382

Open
gabrielbazan7 wants to merge 3 commits into
bitpay:developfrom
gabrielbazan7:fix/push-notification-open-tx2
Open

gabrielbazan7 wants to merge 3 commits into
bitpay:developfrom
gabrielbazan7:fix/push-notification-open-tx2

Conversation

@gabrielbazan7

Copy link
Copy Markdown
Collaborator

No description provided.

@JohnathanWhite JohnathanWhite left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verdict: shippable as is. Nothing here is a regression, and the core change is sound — the polling arithmetic is right (6 × 500ms = 3000ms, and the spec pins it), case is normalised on both sides of the match, the non-token path is untouched, and re-reading getState().WALLET.keys[keyId] for the key param is the right call so the freshly created token wallet is actually in the object you navigate with. Shipping tests with it is appreciated.

Two things worth a look, neither blocking.

1. selectedAccountAddress can be undefined on the new fallback path.

app.effects.ts passes wallet.receiveAddress into ACCOUNT_DETAILS, but receiveAddress?: string is optional on the Wallet model (wallet.models.ts:148), while AccountDetails declares the param as required — selectedAccountAddress: string (AccountDetails.tsx:190). It then resolves the account by equality:

(w: Wallet) => w.receiveAddress === selectedAccountAddress   // AccountDetails.tsx:476, and :532

So if receiveAddress is unset, undefined === undefined matches the first wallet that also has no address — an arbitrary account — rather than failing cleanly.

How reachable: receiveAddress is persisted (the WALLET blacklist is empty) and survives rehydration, since bootstrapWallets merges onto the persisted object rather than rebuilding from credentials. So for anyone who has used the wallet, it's populated. The gap is a brand-new account whose first activity is the incoming token transfer — which is exactly the scenario this PR exists to handle, so it isn't far-fetched.

Also worth noting the new spec masks it: baseWallet gains receiveAddress: '0xBaseAddress', so the fallback is only ever exercised with the address present. A guard (fall back to the old base-wallet behaviour, or bail) plus a case with receiveAddress undefined would close it.

2. Pre-existing, not from this PR: the identifier match lowercases case-sensitive chains.

const tokenWalletId = `${wallet.credentials.walletId}-${tokenAddress}`.toLowerCase();
w.credentials.walletId.toLowerCase() === tokenWalletId

That line is unchanged by this PR — it only moved — so it is explicitly not a blocker here. But it's fine for EVM contract addresses and wrong in principle for Solana mint addresses, which are case-sensitive Base58. In practice a misroute needs two SVM mints in one wallet differing only by case, so it is close to theoretical.

What makes it worth a separate ticket is the inconsistency rather than the risk: status.ts:339 compares the same identifier case-sensitively

`${bStatus.walletId}-${bStatus.tokenAddress}` === wallet.id

Two places deriving the same key with different normalisation is the kind of thing that bites later. Happy to file it if you'd rather keep this PR focused.

@JohnathanWhite JohnathanWhite left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Two inline notes to go with the review above — neither blocking.

Comment thread src/store/app/app.effects.ts Outdated
Comment thread src/store/app/app.effects.ts Outdated
@gabrielbazan7
gabrielbazan7 force-pushed the fix/push-notification-open-tx2 branch from d08534a to a7bdd7b Compare September 10, 2026 19:53
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.

2 participants