Conversation
The shared start test called getAddressAtIndex in a loop to verify precalculated addresses. This fails on the service facade because it resolves via HTTP API (walletApi.getAddresses), which is not available in this test context. Moved the verification to a dedicated fullnode-specific test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a test-adapter abstraction and two concrete adapters (Fullnode and Service), plus shared and adapter-specific integration tests. Provides standardized types/interfaces for wallet lifecycle, creation, funding, and transaction utilities used across integration suites. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Adapter as IWalletTestAdapter
participant Facade as Wallet Facade
participant Storage as Storage/Network
participant Genesis as Genesis Helper
Test->>Adapter: suiteSetup()
Adapter->>Genesis: init/start
Adapter->>Storage: configure/prepare
Test->>Adapter: createWallet(opts)
Adapter->>Adapter: buildWalletInstance(opts)
Adapter->>Facade: instantiate (seed/xpub/..., config)
Adapter->>Facade: startWallet(pin/password)
Facade->>Storage: load/sync history
Adapter->>Adapter: waitForReady(wallet)
Adapter-->>Test: CreateWalletResult
Test->>Adapter: injectFunds(dest, addr, amount)
Adapter->>Genesis: send funds
Genesis->>Facade: submit tx
Facade->>Storage: update UTXO/state
Adapter-->>Test: txId
Test->>Adapter: stopAllWallets()
Adapter->>Facade: wallet.stop() for each
Facade->>Storage: cleanup
Adapter->>Genesis: suiteTeardown()/cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
__tests__/integration/fullnode-specific/start.test.ts (3)
196-200: Minor: Considerfor...ofwith entries for array iteration.Line 174 uses the cleaner
for...ofwithentries(), while lines 196-200 usefor...in. For consistency and to avoid string-to-number coercion:- for (const addressIndex in walletData.addresses) { - const precalcAddress = walletData.addresses[+addressIndex]; - const addressAtIndex = await hWallet.getAddressAtIndex(+addressIndex); + for (const [index, precalcAddress] of walletData.addresses.entries()) { + const addressAtIndex = await hWallet.getAddressAtIndex(index); expect(precalcAddress).toEqual(addressAtIndex); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/fullnode-specific/start.test.ts` around lines 196 - 200, The loop uses for...in over walletData.addresses causing string index coercion; change it to a for...of with entries() to iterate indexes and values consistently (e.g., iterate over walletData.addresses.entries()), and use the numeric index to call hWallet.getAddressAtIndex(index) and compare the value to avoid +addressIndex coercion; update the block around walletData.addresses and hWallet.getAddressAtIndex accordingly.
246-247: Consider replacingdelay(1000)with deterministic wait.The
delay(1000)before stopping the wallet may lead to flaky tests if the token creation transaction hasn't fully propagated. Consider using a transaction wait helper instead:- await delay(1000); + // Wait for the token creation tx to be fully processed + await waitForTxReceived(hWallet, tokenUid);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/fullnode-specific/start.test.ts` around lines 246 - 247, Replace the fixed sleep with a deterministic wait for the token creation transaction to be confirmed: instead of await delay(1000), await the transaction confirmation using the transaction hash returned by your token creation step (e.g., tokenTxHash) via your existing helper (e.g., await waitForTransaction(tokenTxHash) or await provider.waitForTransaction(tokenTxHash)) before calling await hWallet.stop({ cleanStorage: true, cleanAddresses: true }); this ensures the token creation is fully propagated prior to stopping the wallet.
346-364: Use consistent network configuration in externally signed wallet tests.Lines 349 and 369 use
new Network('privatenet')for HD key derivation, butgenerateWalletHelperconnects toNETWORK_NAME('testnet'). Align these tests with the xpub readonly wallet test (line 283), which correctly uses'testnet':Suggested change
- const rootXpriv = code.toHDPrivateKey('', new Network('privatenet')); + const rootXpriv = code.toHDPrivateKey('', new Network(NETWORK_NAME));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/fullnode-specific/start.test.ts` around lines 346 - 364, The test constructs HD keys with new Network('privatenet') but generateWalletHelper connects to NETWORK_NAME ('testnet'), causing inconsistent network configuration; update the Network instantiation used to derive rootXpriv (the call to new Network(...) used with Mnemonic and toHDPrivateKey in this test) to use 'testnet' (matching the xpub readonly wallet test) so xpriv/xpub and generateWalletHelper operate on the same network; ensure any other Network(...) calls in this test also use 'testnet' for consistency.__tests__/integration/adapters/fullnode.adapter.ts (1)
103-116: Non-started wallet added to tracking array.
buildWalletInstanceadds the wallet tostartedWallets(line 108) even though the wallet hasn't been started. IfstopAllWallets()is called and the wallet was never started, callingstop()on it might throw or behave unexpectedly.This appears intentional based on the pattern (validation tests use
buildWalletInstancethenstartWallet), but the namingstartedWalletsis misleading. Consider either:
- Renaming to
trackedWalletsormanagedWallets- Only adding to the list when
startWalletis called- Adding a comment explaining the rationale
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/adapters/fullnode.adapter.ts` around lines 103 - 116, The buildWalletInstance function is pushing non-started wallets into startedWallets which is misleading; either rename startedWallets to trackedWallets (or managedWallets) across the module (update all references like stopAllWallets and any places assuming "started" semantics) or move the push out of buildWalletInstance and into startWallet so only actually-started HathorWallet instances are added (also ensure stopAllWallets guards against non-started wallets by checking a started state before calling stop). Choose one approach and apply the consistent rename or relocation of the push and update all references to the collection and any stop logic accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/integration/adapters/service.adapter.ts`:
- Around line 155-158: injectFundsBeforeStart currently uses a non-null
assertion on fundTx.hash (from GenesisWalletServiceHelper.injectFunds) without
verifying it exists; update injectFundsBeforeStart to check that fundTx and
fundTx.hash are defined (e.g., if (!fundTx?.hash) throw or return a clear error)
before returning the hash to match the guard behavior in
FullnodeTestAdapter.injectFundsBeforeStart and avoid the unsafe `!` assertion.
In `@__tests__/integration/service-specific/start.test.ts`:
- Around line 90-113: Add a guard assertion and clarify the credential mismatch:
assert accessData.acctPathKey is defined (e.g.,
expect(accessData.acctPathKey).toBeDefined()) before calling
decryptData(accessData.acctPathKey!, '1234') to avoid the non-null assertion
risk, and either align the test credentials or add an inline comment near
generateAccessDataFromSeed / HathorWalletServiceWallet construction explaining
that the '1234' pin/password are only used for key derivation/decryption while
wallet.start(pinCode, password) uses the separate outer-scope credentials
(pinCode/password) to avoid confusion when reading the test.
---
Nitpick comments:
In `@__tests__/integration/adapters/fullnode.adapter.ts`:
- Around line 103-116: The buildWalletInstance function is pushing non-started
wallets into startedWallets which is misleading; either rename startedWallets to
trackedWallets (or managedWallets) across the module (update all references like
stopAllWallets and any places assuming "started" semantics) or move the push out
of buildWalletInstance and into startWallet so only actually-started
HathorWallet instances are added (also ensure stopAllWallets guards against
non-started wallets by checking a started state before calling stop). Choose one
approach and apply the consistent rename or relocation of the push and update
all references to the collection and any stop logic accordingly.
In `@__tests__/integration/fullnode-specific/start.test.ts`:
- Around line 196-200: The loop uses for...in over walletData.addresses causing
string index coercion; change it to a for...of with entries() to iterate indexes
and values consistently (e.g., iterate over walletData.addresses.entries()), and
use the numeric index to call hWallet.getAddressAtIndex(index) and compare the
value to avoid +addressIndex coercion; update the block around
walletData.addresses and hWallet.getAddressAtIndex accordingly.
- Around line 246-247: Replace the fixed sleep with a deterministic wait for the
token creation transaction to be confirmed: instead of await delay(1000), await
the transaction confirmation using the transaction hash returned by your token
creation step (e.g., tokenTxHash) via your existing helper (e.g., await
waitForTransaction(tokenTxHash) or await
provider.waitForTransaction(tokenTxHash)) before calling await hWallet.stop({
cleanStorage: true, cleanAddresses: true }); this ensures the token creation is
fully propagated prior to stopping the wallet.
- Around line 346-364: The test constructs HD keys with new
Network('privatenet') but generateWalletHelper connects to NETWORK_NAME
('testnet'), causing inconsistent network configuration; update the Network
instantiation used to derive rootXpriv (the call to new Network(...) used with
Mnemonic and toHDPrivateKey in this test) to use 'testnet' (matching the xpub
readonly wallet test) so xpriv/xpub and generateWalletHelper operate on the same
network; ensure any other Network(...) calls in this test also use 'testnet' for
consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5a59f60-24c5-4ead-b156-073907d20946
📒 Files selected for processing (8)
__tests__/integration/adapters/fullnode.adapter.ts__tests__/integration/adapters/service.adapter.ts__tests__/integration/adapters/types.ts__tests__/integration/fullnode-specific/start.test.ts__tests__/integration/hathorwallet_facade.test.ts__tests__/integration/service-specific/start.test.ts__tests__/integration/shared/start.shared.ts__tests__/integration/walletservice_facade.test.ts
💤 Files with no reviewable changes (1)
- tests/integration/walletservice_facade.test.ts
Orphaned imports left behind after extracting start() tests into the shared test architecture. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1032 +/- ##
==========================================
+ Coverage 87.77% 87.86% +0.09%
==========================================
Files 114 114
Lines 8828 8828
Branches 2006 2006
==========================================
+ Hits 7749 7757 +8
+ Misses 1051 1043 -8
Partials 28 28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
__tests__/integration/adapters/fullnode.adapter.ts (1)
173-175: Non-null assertion onprecalculationHelpers.testassumes prior initialization.The
precalculationHelpers.test!assertion assumes the test helper was initialized during test setup. This is a reasonable assumption for test infrastructure, but ifgetPrecalculatedWallet()is ever called before suite setup completes, it will throw a cryptic runtime error.Consider adding a guard for clearer error messaging:
🛡️ Optional defensive check
getPrecalculatedWallet(): PrecalculatedWalletData { + if (!precalculationHelpers.test) { + throw new Error('Precalculation helper not initialized — was suiteSetup() called?'); + } - return precalculationHelpers.test!.getPrecalculatedWallet(); + return precalculationHelpers.test.getPrecalculatedWallet(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/adapters/fullnode.adapter.ts` around lines 173 - 175, The method getPrecalculatedWallet() currently uses a non-null assertion on precalculationHelpers.test which will throw a cryptic error if not initialized; update getPrecalculatedWallet (and any similar accessors) to first check if precalculationHelpers.test is defined and, if not, throw a clear, descriptive error (e.g., "precalculationHelpers.test is not initialized - ensure test setup ran") or return a safe fallback; reference the getPrecalculatedWallet function and the precalculationHelpers.test symbol when adding this guard so callers get a deterministic, debuggable failure instead of a runtime assertion error.__tests__/integration/adapters/service.adapter.ts (1)
167-169: Same non-null assertion pattern as fullnode adapter.Same optional defensive check could be applied here as suggested for
FullnodeTestAdapter.getPrecalculatedWallet().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/adapters/service.adapter.ts` around lines 167 - 169, The getPrecalculatedWallet method currently uses a non-null assertion on precalculationHelpers.test; change it to defensively check for precalculationHelpers.test (same approach as FullnodeTestAdapter.getPrecalculatedWallet) and either throw a clear error or return a safe fallback when it's undefined, then call .getPrecalculatedWallet() only when the test helper exists; reference symbols: getPrecalculatedWallet, PrecalculatedWalletData, precalculationHelpers.test, and FullnodeTestAdapter.getPrecalculatedWallet.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@__tests__/integration/adapters/fullnode.adapter.ts`:
- Around line 173-175: The method getPrecalculatedWallet() currently uses a
non-null assertion on precalculationHelpers.test which will throw a cryptic
error if not initialized; update getPrecalculatedWallet (and any similar
accessors) to first check if precalculationHelpers.test is defined and, if not,
throw a clear, descriptive error (e.g., "precalculationHelpers.test is not
initialized - ensure test setup ran") or return a safe fallback; reference the
getPrecalculatedWallet function and the precalculationHelpers.test symbol when
adding this guard so callers get a deterministic, debuggable failure instead of
a runtime assertion error.
In `@__tests__/integration/adapters/service.adapter.ts`:
- Around line 167-169: The getPrecalculatedWallet method currently uses a
non-null assertion on precalculationHelpers.test; change it to defensively check
for precalculationHelpers.test (same approach as
FullnodeTestAdapter.getPrecalculatedWallet) and either throw a clear error or
return a safe fallback when it's undefined, then call .getPrecalculatedWallet()
only when the test helper exists; reference symbols: getPrecalculatedWallet,
PrecalculatedWalletData, precalculationHelpers.test, and
FullnodeTestAdapter.getPrecalculatedWallet.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9e861b1-69ec-49f9-a505-df92aa5c5e9d
📒 Files selected for processing (6)
__tests__/integration/adapters/fullnode.adapter.ts__tests__/integration/adapters/service.adapter.ts__tests__/integration/fullnode-specific/start.test.ts__tests__/integration/service-specific/start.test.ts__tests__/integration/shared/start.shared.ts__tests__/integration/walletservice_facade.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/walletservice_facade.test.ts
Replace the exported registerSharedStartTests() factory with a self-contained describe.each test file that Jest discovers directly. This eliminates the need for manual imports or registration in facade files and removes the jest/no-export lint suppression.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/integration/shared/start.test.ts (1)
113-148: Clarify non-standard parameter order ingetRandomIntusage.The call
getRandomInt(10, 1)uses a non-standard argument order (max,min), whereas most random utilities follow the convention of (min,max). While the code is correct, adding a brief comment would help future readers understand the intentional parameter order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/shared/start.test.ts` around lines 113 - 148, The test uses getRandomInt(10, 1) with a non-standard (max, min) argument order; update the test "should start with a transaction history" to add a short inline comment next to the injectValue assignment referencing getRandomInt to clarify the intentional parameter order (e.g., note that getRandomInt accepts (max, min) here) so future readers aren't confused; keep the change local to this test and do not alter getRandomInt implementation or other calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@__tests__/integration/shared/start.test.ts`:
- Around line 113-148: The test uses getRandomInt(10, 1) with a non-standard
(max, min) argument order; update the test "should start with a transaction
history" to add a short inline comment next to the injectValue assignment
referencing getRandomInt to clarify the intentional parameter order (e.g., note
that getRandomInt accepts (max, min) here) so future readers aren't confused;
keep the change local to this test and do not alter getRandomInt implementation
or other calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43807ec4-e5fa-4833-a5e9-c21aedbf80c5
📒 Files selected for processing (3)
__tests__/integration/fullnode-specific/start.test.ts__tests__/integration/service-specific/start.test.ts__tests__/integration/shared/start.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/service-specific/start.test.ts
r4mmer
reviewed
Mar 13, 2026
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/integration/adapters/fullnode.adapter.ts`:
- Around line 207-212: The current fillDefaults branch always overwrites
provided credentials with DEFAULT_PASSWORD and DEFAULT_PIN_CODE; change the
logic so defaults are applied only when the caller left fields undefined: when
fillDefaults is true, merge in password: options?.password === undefined ?
DEFAULT_PASSWORD : options.password and pinCode: options?.pinCode === undefined
? DEFAULT_PIN_CODE : options.pinCode (preserving existing behavior when options
is undefined), referencing the fillDefaults flag, DEFAULT_PASSWORD,
DEFAULT_PIN_CODE, options, password and pinCode in the create path so explicit
credentials are never silently replaced.
- Around line 91-95: The test constructs a HathorWallet but only adds it to
this.trackedWallets after hWallet.start() and waitForWalletReady(), so if
startup throws the wallet isn't tracked and stopAllWallets can't clean it; fix
by pushing the new HathorWallet instance into this.trackedWallets immediately
after creating hWallet (before calling hWallet.start() and
waitForWalletReady()), and wrap the start/wait calls in a try/catch if you want
to log and rethrow while still keeping the wallet tracked so stopAllWallets can
handle cleanup (refer to HathorWallet, start(), waitForWalletReady(),
this.trackedWallets, and stopAllWallets).
In `@__tests__/integration/fullnode-specific/start.test.ts`:
- Around line 57-59: Tests leak wallets because some HathorWallet instances are
created and started outside the adapter lifecycle; ensure each manual wallet
lifecycle either registers the wallet with adapter (so adapter.stopAllWallets()
will clean it up) or wrap the start/usage/stop sequence in try/finally to call
wallet.stop() on all execution paths. Locate the manual new HathorWallet(...)
usages referenced (the blocks at lines ~242-269, ~279-356, ~370-379) and for
each: either call adapter.registerWallet(wallet) immediately after creation, or
surround the code that starts/uses the wallet with try { ... } finally { await
wallet.stop(); } so failures don’t leave live connections/listeners; keep
afterEach(async () => await adapter.stopAllWallets()) as the global safety net.
In `@__tests__/integration/shared/start.test.ts`:
- Line 17: The file's formatting is failing CI for the adapters declaration; run
Prettier (or the project's formatter) on the test file and reformat the
declaration using the project's style rules so the line declaring "const
adapters: IWalletTestAdapter[] = [new FullnodeWalletTestAdapter(), new
ServiceWalletTestAdapter()];" matches the repository formatting settings (ensure
spacing, trailing commas, and line breaks conform to configured Prettier rules).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d02af521-d797-4797-b94d-75e67f144cf1
📒 Files selected for processing (5)
__tests__/integration/adapters/fullnode.adapter.ts__tests__/integration/adapters/service.adapter.ts__tests__/integration/fullnode-specific/start.test.ts__tests__/integration/service-specific/start.test.ts__tests__/integration/shared/start.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/service-specific/start.test.ts
- tests/integration/adapters/service.adapter.ts
| capabilities: WalletCapabilities = { | ||
| supportsMultisig: false, | ||
| supportsTokenScope: false, | ||
| supportsXpubReadonly: false, |
Member
There was a problem hiding this comment.
The wallet service has readonly wallet. Is it different?
Contributor
Author
There was a problem hiding this comment.
We didn't have tests for it yet. I thought about postponing this to another PR but decided to implement it right now, as it's a good opportunity to increase coverage. Done on b362c67
Addresses PR review comment: stop options were duplicated between the WalletTracker constructor and the stopWallet method in both adapters. Now both reference a single STOP_OPTIONS constant. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addresses PR review comment: explains that injectFunds polls the destination wallet for tx confirmation, which would fail when the wallet hasn't started yet. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addresses PR review comment: documents that words is intentionally undefined when only xpub or xpriv is provided, since buildConfig handles those fields independently of the seed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addresses PR review comments #2 and #3: createWallet no longer duplicates wallet construction logic. It delegates to buildWalletInstance for construction and startWallet for startup, passing default credentials at start() time. This also removes the fillDefaults flag from buildConfig since credentials are now always provided at start time rather than construction time. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Enables readonly wallet testing for the wallet-service facade and reorganizes readonly tests into shared vs. facade-specific: - Service adapter: adds xpub support in buildWalletInstance/startWallet, routing to startReadOnly() for xpub wallets. Pre-registers the wallet on the backend (via seed start+stop) before attaching as readonly. - Shared tests: isReadonly verification, zero balance, fund injection — run against both facades via describe.each. - Fullnode-specific: address generation (local derivation) and 15 WalletFromXPubGuard assertions. - Service-specific: 5 WalletFromXPubGuard assertions for guarded methods. - Helper: buildWalletInstance() now accepts optional xpub parameter. Addresses PR review comment #1 about wallet-service readonly support. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Deduplicates the 3-line Mnemonic→xpub derivation that was copy-pasted across shared, fullnode-specific, and service-specific test files into a single helper in core.util.ts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addresses CodeRabbit review findings: - Replace unsafe (e as Error).message casts with instanceof checks - Add expect(addr).toBeDefined() before non-null assertion in readonly fund injection test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
r4mmer
approved these changes
Mar 25, 2026
pedroferreira1
approved these changes
Mar 25, 2026
github-project-automation
bot
moved this from In Progress (Done)
to In Review (WIP)
in Hathor Network
github-project-automation
bot
moved this from In Review (WIP)
to Waiting to be deployed
in Hathor Network
This was referenced Mar 26, 2026
raul-oliveira
added a commit
that referenced
this pull request
Apr 10, 2026
* feat: export all types and utils from public API (#1034) * Test: Shared tests for `start` (#1032) * Merge pull request #1046 from HathorNetwork/refact/fee-test-suite refact: move fee test suite to the right folder * test: add new fee template integration tests (#1048) * refact: move fee test suite to the right folder * review changes * refact: keep only moved tests, remove new template tests * test: add new fee template integration tests * fix: native token version defaulting to deposit (#1054) * test: Shared tests for internal wallet methods (#1047) BREAKING CHANGE: HathorWallet.changeServer now returns Promise<void> instead of void, matching the service facade signature. Added changeServer to IHathorWallet. * fix: precalculated wallet address derivation bug (#1056) * feat: single address policy (#1038) * feat: single address policy * chore: improve CI integration test logs (#1049) --------- Co-authored-by: André Abadesso <andre.abadesso@gmail.com> Co-authored-by: Tulio Miranda <tulio.mir@gmail.com> Co-authored-by: André Carneiro <andreluizmrcarneiro@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces a shared integration test architecture for the
start()method across both wallet facades (HathorWalletandHathorWalletServiceWallet), serving as both an implementation and a Proof of Concept for the testing strategy going forward.Motivation
Today, the two wallet facades have independent, largely duplicated test suites with no shared validation point. This makes it easy for behavioral drift to go unnoticed and makes it harder to reason about what the facades guarantee as a contract. We need shared tests — but the facades (and their supposed shared interface,
IHathorWallet) are not yet aligned. This is being addressed by HathorNetwork/rfcs#100.While the changes proposed by RFC 100 are implemented, this PR takes a pragmatic approach: develop test adapters that bridge the current API mismatches, letting us write shared tests today. These adapters are explicitly designed as a temporary layer — they can be trimmed down or removed entirely once RFC 100 unifies the facade interfaces.
What this PR does
1. Adapter layer (
__tests__/integration/adapters/)IWalletTestAdapter— a unified interface for test setup, wallet lifecycle, fund injection, and capability queries.FullnodeTestAdapterandServiceTestAdapter— concrete implementations that wrap each facade's existing test helpers, normalizing differences (e.g. fullnode's non-blockingstart()vs service's blockingstart()).WalletCapabilities— feature flags (supportsMultisig,requiresExplicitWaitReady, etc.) so shared tests can conditionally skip unsupported scenarios instead of failing.2. Shared tests via
describe.each(__tests__/integration/shared/)start.test.ts— a self-contained test file that uses Jest'sdescribe.eachto runstart()behavior tests against both adapters: parameter validation, successful startup with empty/existing history, and state event ordering.3. Per-feature file splitting (
fullnode-specific/,service-specific/)hathorwallet_facade.test.tsandwalletservice_facade.test.tsare being broken apart into smaller, feature-scoped files.start.test.tsfor both facades; further features will follow.4. Deduplication
Design decisions
FuzzyWalletTypeunion and the casts it requires are a deliberate signal of this misalignment.describe.eachover factory exports: Shared tests usedescribe.each([adapters])so they are proper.test.tsfiles auto-discovered by Jest and following the structures offered by this test framework.Related RFCs
File summary
adapters/types.tsadapters/fullnode.adapter.tsadapters/service.adapter.tsshared/start.test.tsstart()tests viadescribe.eachfullnode-specific/start.test.tsservice-specific/start.test.tshathorwallet_facade.test.tswalletservice_facade.test.tsTest plan
fullnode-specific/start.test.ts)service-specific/start.test.ts)shared/start.test.ts)Summary by CodeRabbit