Skip to content

LIVE-25828: coin-bitcoin use observable event stream#14732

Closed
pvoliveira wants to merge 1 commit intodevelopfrom
feat/coin-bitcoin-use-observable-event-stream
Closed

LIVE-25828: coin-bitcoin use observable event stream#14732
pvoliveira wants to merge 1 commit intodevelopfrom
feat/coin-bitcoin-use-observable-event-stream

Conversation

@pvoliveira
Copy link
Contributor

@pvoliveira pvoliveira commented Feb 24, 2026

✅ Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • coin-bitcoin: synchronisation (no changes in the current behaviour)

📝 Description

Refactor makeGetAccountShape in coin-bitcoin so it returns a synchronous function that yields an Observable emitting the account shape (one value then complete).

Note: this PR is based on the changes from #14274.

❓ Context


🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

@pvoliveira pvoliveira requested a review from a team as a code owner February 24, 2026 11:07
Copilot AI review requested due to automatic review settings February 24, 2026 11:07
@pvoliveira pvoliveira changed the title Feat/coin bitcoin use observable event stream LIVE-25828: coin-bitcoin use observable event stream Feb 24, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Feb 24, 2026

⚠️ E2E tests are required

Changes detected require e2e testing before merge (even before asking for any review).

🖥️ Desktop

-> Run Desktop E2E

  • Select "Run workflow"
  • Branch: feat/coin-bitcoin-use-observable-event-stream
  • Device: nanoSP or stax

📱 Mobile

-> Run Mobile E2E

  • Select "Run workflow"
  • Branch: feat/coin-bitcoin-use-observable-event-stream
  • Device: nanoX

Affected coins modules: bitcoin

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the makeGetAccountShape function in coin-bitcoin to return an Observable instead of a Promise, making it the first coin module to adopt the new Observable-based synchronization pattern introduced in PR #14274. This change supports future async synchronization requirements for ZCash Shielded transactions while maintaining backward compatibility through the framework's normalization layer.

Changes:

  • Refactored coin-bitcoin's makeGetAccountShape to return GetAccountShapeStream<BitcoinAccount> (Observable) instead of GetAccountShape<BitcoinAccount> (Promise)
  • Added framework support in jsHelpers.ts for both Promise and Observable-based getAccountShape implementations with automatic normalization
  • Extended test coverage to verify Observable behavior including emission patterns, error handling, and subscription cleanup

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
libs/coin-modules/coin-bitcoin/src/synchronisation.ts Extracted async logic into getAccountShapeAsync and wrapped it with from() Observable in makeGetAccountShape; removed unnecessary await on synchronous getUniquesAddresses call
libs/coin-modules/coin-bitcoin/src/tests/unit/synchronisation.unit.test.ts Added tests for Observable behavior: error propagation, single-value emission, and completion
libs/coin-framework/src/bridge/jsHelpers.ts Added GetAccountShapeStream type, normalizeToObservable helper, updated makeSync and makeScanAccounts to support both Promise and Observable patterns with proper subscription management
libs/coin-framework/src/bridge/jsHelpers.test.ts Added comprehensive tests for Observable support in makeSync and makeScanAccounts including multi-emission, subscription cleanup, and error handling
.changeset/small-terms-learn.md Changeset documenting framework Observable support
.changeset/red-students-drum.md Changeset documenting coin-bitcoin migration to Observable pattern
Comments suppressed due to low confidence (1)

.changeset/red-students-drum.md:5

  • The changeset description says "makescanaccountshape" but the function is actually called "makeGetAccountShape" (not "makeScanAccountShape"). This should be corrected to accurately describe the change.
change makescanaccountshape to use the new observable approach

@pvoliveira pvoliveira force-pushed the feat/coin-bitcoin-use-observable-event-stream branch 2 times, most recently from 49f7e69 to 2bab0be Compare February 24, 2026 12:34
Copilot AI review requested due to automatic review settings February 24, 2026 12:34
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

@pvoliveira pvoliveira force-pushed the feat/coin-bitcoin-use-observable-event-stream branch 3 times, most recently from 2b496e7 to 9b9de6b Compare February 24, 2026 17:52
Copilot AI review requested due to automatic review settings February 24, 2026 17:52
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (2)

libs/coin-framework/src/bridge/jsHelpers.test.ts:600

  • This test intends to assert that all discovered events are emitted before complete, but it never records the completion event in events. As written, lastEvent.type will always be "discovered" regardless of completion ordering. Push a sentinel entry in complete before resolving (or track a completed flag and assert no next happens after it).
    await new Promise<void>((resolve, reject) => {
      scanAccounts({
        currency,
        deviceId: "deviceId",
        syncConfig: { paginationConfig: {} },
      }).subscribe({
        next: event => events.push(event),
        complete: () => resolve(),
        error: reject,
      });

.changeset/small-terms-learn.md:5

  • Grammar in changeset summary: use "add support for observables" (or "add Observable support") instead of "add support to observables".
add support to observables for makeSync and makeScanAccounts

Comment on lines +553 to +555
const account$ = stepAccount(index, res, derivationMode, seedIdentifier);
const account = await lastValueFrom(account$);

Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

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

lastValueFrom(account$) subscribes to the inner getAccountShape Observable but there is no way to cancel that subscription when the outer scanAccounts subscription is unsubscribed (it only flips finished). This can keep network/device work running after cancellation and can hang the scan if the inner Observable never completes. Consider wiring cancellation into the inner stream (e.g. takeUntil fed by the outer teardown) and/or prefer firstValueFrom if only the first shape is needed.

Copilot uses AI. Check for mistakes.
@pvoliveira pvoliveira force-pushed the feat/coin-bitcoin-use-observable-event-stream branch from 9b9de6b to 6a4e48d Compare February 25, 2026 12:22
Copilot AI review requested due to automatic review settings February 26, 2026 12:46
@pvoliveira pvoliveira force-pushed the feat/coin-bitcoin-use-observable-event-stream branch from 6a4e48d to 7e458a6 Compare February 26, 2026 12:46
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

libs/coin-modules/coin-bitcoin/src/tests/unit/synchronisation.unit.test.ts:6

  • BitcoinAccount is only used in a type position here; in this test suite the convention is to use import type for type-only imports (e.g. signOperation.test.ts). Switching this to a type-only import avoids emitting an unnecessary runtime import and keeps consistency across the tests.
import { createFixtureAccount, mockSignerContext } from "../../fixtures/common.fixtures";
import { BitcoinAccount } from "../../types";

@pvoliveira pvoliveira force-pushed the feat/coin-bitcoin-use-observable-event-stream branch from 7e458a6 to 0e755e5 Compare February 26, 2026 17:22
Copilot AI review requested due to automatic review settings February 27, 2026 12:52
@pvoliveira pvoliveira force-pushed the feat/coin-bitcoin-use-observable-event-stream branch from 0e755e5 to 31cf405 Compare February 27, 2026 12:52
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

.changeset/small-terms-learn.md:5

  • Changeset summary reads a bit ungrammatical (“add support to observables…”). Consider rephrasing to “add support for Observables in makeSync and makeScanAccounts” to be clearer in release notes.
add support to observables for makeSync and makeScanAccounts

@pvoliveira pvoliveira force-pushed the feat/coin-bitcoin-use-observable-event-stream branch from 31cf405 to db0cbdd Compare February 27, 2026 13:57
Copilot AI review requested due to automatic review settings February 27, 2026 14:54
@pvoliveira pvoliveira force-pushed the feat/coin-bitcoin-use-observable-event-stream branch from db0cbdd to f84e544 Compare February 27, 2026 14:54
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

@pvoliveira pvoliveira force-pushed the feat/coin-bitcoin-use-observable-event-stream branch from f84e544 to 88a51ff Compare March 2, 2026 10:34
Copilot AI review requested due to automatic review settings March 2, 2026 11:09
@pvoliveira pvoliveira force-pushed the feat/coin-bitcoin-use-observable-event-stream branch from 88a51ff to b3d22e0 Compare March 2, 2026 11:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

.changeset/small-terms-learn.md:5

  • Changeset text is grammatically off: "add support to observables" reads like you’re adding support to observables rather than for observables. Consider rewording to something like "add support for Observables in makeSync and makeScanAccounts" for clarity.
add support to observables for makeSync and makeScanAccounts

@pvoliveira pvoliveira force-pushed the feat/coin-bitcoin-use-observable-event-stream branch from b3d22e0 to 4b39687 Compare March 2, 2026 17:32
Copilot AI review requested due to automatic review settings March 4, 2026 10:15
@pvoliveira pvoliveira force-pushed the feat/coin-bitcoin-use-observable-event-stream branch from 4b39687 to 6ef1156 Compare March 4, 2026 10:15
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

@pvoliveira pvoliveira force-pushed the feat/coin-bitcoin-use-observable-event-stream branch from 6ef1156 to 39c2083 Compare March 4, 2026 10:34
@pvoliveira pvoliveira closed this Mar 4, 2026
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 4, 2026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants