Ref D20189
Here we init the actual server with its CI tests and build
We will implement the notifications in Cashtab in a follow-up diff
Details
Details
- Reviewers
Fabien - Group Reviewers
Restricted Project - Commits
- rABC5f2bd5adbc11: [notifications-server] Init notifications-server for Cashtab push notifications…
npm test
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| apps/notifications-server/package.json | ||
|---|---|---|
| 30 ↗ | (On Diff #59992) | This should not be necessary, cors will be set by the reverse proxy |
| apps/notifications-server/src/app.ts | ||
| 20 ↗ | (On Diff #59992) | see comment above |
| apps/notifications-server/src/pushTxParse.test.ts | ||
| 14 ↗ | (On Diff #59992) | Can you explain what this is ? |
| apps/notifications-server/src/routes/push.test.ts | ||
| 92 ↗ | (On Diff #59992) | you should test unregistering an unsigned token |
| apps/notifications-server/src/routes/push.ts | ||
| 62 ↗ | (On Diff #59992) | I don't think this PUSH_REGISTER_AUTH_MESSAGE is a good idea. Even if this is not critical security-wise, you should always sign what you're committing to. In this case you can simply sign the wallet address itself. This makes the message different for each address which is a nice improvement for security and it simplifies the code |
| apps/notifications-server/src/services/pushTokenStore.ts | ||
| 25 ↗ | (On Diff #59992) | Nice, so updated_at is refreshed everytime the device is attempting to register. This means that we can determine is a subscription has been inactive for a while and delete it to avoid growing the db indefinitely |
| apps/notifications-server/src/services/pushTxNotify.ts | ||
| 5 ↗ | (On Diff #59992) | Is that really necessary ? |
| apps/notifications-server/src/websockets/pushAddressWs.ts | ||
| 55 ↗ | (On Diff #59992) | I suggest you print the desired.length to the console before listing all the addresses, so it's easy to get a sense of how much address are initially subscribed without counting the lines |
| 71 ↗ | (On Diff #59992) | This is missing the transactions that were not discovered before being mined. E.g. this will miss the staking rewards |
| 83 ↗ | (On Diff #59992) | This works but it's inefficient. subscribedAddresses is a set so random access is cheap, which means that for the few addresses (most likely) of the tx you are parsing all the subscriptions while you could instead just compute a few cheap lookups. You expect the number of address in each tx to be << number of subscriptions. |
| 86 ↗ | (On Diff #59992) | General review note: I expect it to be kinda difficult to review for people that know nothing about firebase as the word token has a different meaning depending on the context. Maybe consistently use fcm_token for firebase ? |
| notifications-server.Dockerfile | ||
| 76 ↗ | (On Diff #59992) | style nit: you can use --filter several times (and use a line continuation for readability), see ecash-wallet.Dockerfile for reference. |
| apps/notifications-server/package.json | ||
|---|---|---|
| 30 ↗ | (On Diff #59992) | k removed |
| apps/notifications-server/src/pushTxParse.test.ts | ||
| 14 ↗ | (On Diff #59992) | AI trying to be helpful by supporting other stuff. updated; cashtab wallets are all only p2pkh |
| apps/notifications-server/src/services/pushTxNotify.ts | ||
| 5 ↗ | (On Diff #59992) | no, I don't know why AI loves re-exporting things, deleted |
| apps/notifications-server/src/websockets/pushAddressWs.ts | ||
| 71 ↗ | (On Diff #59992) | I think the best pattern here is subscribe to blocks and inspect the coinbase tx; implemented |
| 83 ↗ | (On Diff #59992) | used a tighter approach |
| 86 ↗ | (On Diff #59992) | good point |
Comment Actions
Tail of the build log:
└────────────────────────────────────────────────────────────────────────────────────────────────┘
================================================================================
(Run Finished)
Spec Tests Passing Failing Pending Skipped
┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ ✔ bip21-payment-request.cy.ts 00:10 6 6 - - - │
├────────────────────────────────────────────────────────────────────────────────────────────────┤
│ ✔ history.cy.ts 00:15 10 10 - - - │
├────────────────────────────────────────────────────────────────────────────────────────────────┤
│ ✔ main.cy.ts 00:11 7 7 - - - │
├────────────────────────────────────────────────────────────────────────────────────────────────┤
│ ✔ mnemonic.cy.ts 00:17 7 7 - - - │
├────────────────────────────────────────────────────────────────────────────────────────────────┤
│ ✖ send.cy.ts 01:56 24 23 1 - - │
├────────────────────────────────────────────────────────────────────────────────────────────────┤
│ ✔ shared-image-qr.cy.ts 00:06 5 5 - - - │
├────────────────────────────────────────────────────────────────────────────────────────────────┤
│ ✔ transitional-balance.cy.ts 00:16 9 9 - - - │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
✖ 1 of 7 failed (14%) 03:15 68 67 1 - -
--------------------------------------------------------------------------------
Debug faster with full visibility.
Record to Cypress Cloud and get instant access to full test details and replays.
Inspect the DOM, network events, and console logs exactly as they ran in CI.
>> https://on.cypress.io/cloud-get-started
--------------------------------------------------------------------------------
ELIFECYCLE Command failed with exit code 1.
<i> [webpack-dev-server] Gracefully shutting down. To force exit, press ^C again. Please wait...
Error: Command failed with exit code 1: pnpm run cypress:run
at makeError (/work/node_modules/.pnpm/execa@5.1.1/node_modules/execa/lib/error.js:60:11)
at handlePromise (/work/node_modules/.pnpm/execa@5.1.1/node_modules/execa/index.js:118:26)
at process.processTicksAndRejections (node:internal/process/task_queues:105:5) {
shortMessage: 'Command failed with exit code 1: pnpm run cypress:run',
command: 'pnpm run cypress:run',
escapedCommand: '"pnpm run cypress:run"',
exitCode: 1,
signal: undefined,
signalDescription: undefined,
stdout: undefined,
stderr: undefined,
failed: true,
timedOut: false,
isCanceled: false,
killed: false
}
/work/apps/marlin-wallet/web:
ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL marlin-wallet-web@2.2.1 test:e2e: `start-server-and-test 'pnpm run dev' http://localhost:3000 'pnpm run cypress:run'`
Exit status 1
ELIFECYCLE Command failed.
Build marlin-wallet-e2e-tests failed with exit code 1Comment Actions
Only mined non final txs remain to be fixed because it's not a coinbase only issue, but is more general. Otherwise looks pretty good
| apps/notifications-server/src/services/pushTokenStore.ts | ||
|---|---|---|
| 13 ↗ | (On Diff #60000) |
|
| apps/notifications-server/src/websockets/pushAddressWs.ts | ||
| 71 ↗ | (On Diff #59992) | There can very well be others txs than the coinbase. The staking reward was the obvious example but a miner might get any tx before your chronik node and mine it before it had a chance to finalize at the chronik node |
| apps/notifications-server/src/websockets/pushAddressWs.ts | ||
|---|---|---|
| 71 ↗ | (On Diff #59992) | had not considered this. so if a tx is mined in this way, a chronik node would never expect the preconcensus finalized msg? in theory this could also mean a chronik node could miss the added to the mempool msg in the same way? In that case ... I'm not sure how to solve here without some kind of cache and checking every blockTx to see if we have already seen it. In practice, that kind of system might make sense for a notification server anyway. Just check every single tx that comes in on the network and inspect them for notify-ability, instead of maintaining a complicated set of websocket subscriptions. It would certainly make sense today, where we do not see many txs on the network and parsing them all is straightforward. I think for an MVP tho ... it's okay to just only respond to preconcensus finalized. In practice we expect to miss almost no txs this way, and the consequence of a miss is only a missed notification. |
Comment Actions
Document acceptable miss of pre-consensus notifications when a miner includes a tx before Chronik sees it. Rebased onto latest master.
Comment Actions
Document acceptable miss of pre-consensus notifications when a miner includes a tx before Chronik sees it. Rebased onto latest master.
Comment Actions
Squash to a single commit (rebased on master; document acceptable pre-consensus notification misses).
Comment Actions
Re-upload with lint enabled (no skip). Squashed single commit on master; document acceptable pre-consensus notification misses.
Comment Actions
Re-upload with lint run (not skipped). Single commit on master; document acceptable pre-consensus notification misses.
Comment Actions
Re-upload with lint enabled (do not use --head; it skips lint). Single commit on master; document acceptable pre-consensus notification misses.