Merged
Conversation
|
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:
📝 WalkthroughWalkthroughAdd a SINGLE_ADDRESS scanning policy and enforce it across types, storage, wallet logic, and utilities; update MemoryStore to route index changes through setCurrentAddressIndex with a guard for SINGLE_ADDRESS; add wallet.enableSingleAddressMode() and multiple tests covering single-address behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Wallet
participant Storage
participant MemoryStore
participant Utilities
Client->>Wallet: enableSingleAddressMode()
Wallet->>Storage: setScanningPolicyData(SINGLE_ADDRESS)
Storage->>MemoryStore: persist policy
Client->>Wallet: getCurrentAddress(markAsUsed=true)
alt SINGLE_ADDRESS enabled
Wallet->>Wallet: return cached firstAddress (index 0)
else
Wallet->>Utilities: scanPolicyStartAddresses(storage)
Utilities->>Storage: getScanningPolicyData()
Storage-->>Utilities: policy data
Utilities-->>Wallet: addresses to load
end
Wallet->>Storage: setCurrentAddressIndex(newIndex)
Storage->>MemoryStore: setCurrentAddressIndex(newIndex)
alt policy == SINGLE_ADDRESS and newIndex > 0
MemoryStore->>MemoryStore: early return (no advance)
MemoryStore-->>Storage: no-op
else
MemoryStore->>MemoryStore: update currentAddressIndex
MemoryStore-->>Storage: updated index
end
Storage-->>Wallet: confirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1038 +/- ##
===========================================
+ Coverage 66.53% 87.93% +21.39%
===========================================
Files 114 114
Lines 8828 8910 +82
Branches 2006 2020 +14
===========================================
+ Hits 5874 7835 +1961
+ Misses 2926 1047 -1879
Partials 28 28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/wallet/wallet.ts (2)
410-420:⚠️ Potential issue | 🟠 MajorDon't overwrite the stored scanning policy during
start().
this.scanPolicyonly reflects constructor input and defaults tonull;enableSingleAddressPolicy()also leaves it stale. Startup now clears a persisted single-address policy or reverts it on same-instance restart instead of honoring storage.🔄 Suggested fix
this.setState(walletState.LOADING); - await this.storage.setScanningPolicyData(this.scanPolicy); + if (this.scanPolicy !== null) { + await this.storage.setScanningPolicyData(this.scanPolicy); + } else { + this.scanPolicy = await this.storage.getScanningPolicyData(); + this.singleAddress = isSingleAddressScanPolicy(this.scanPolicy); + }- await this.storage.setScanningPolicyData({ - policy: SCANNING_POLICY.SINGLE_ADDRESS, - }); + this.scanPolicy = { policy: SCANNING_POLICY.SINGLE_ADDRESS }; + await this.storage.setScanningPolicyData(this.scanPolicy);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.ts` around lines 410 - 420, The start() method is overwriting persisted scanning policy by unconditionally calling this.storage.setScanningPolicyData(this.scanPolicy); instead, read the stored policy first (e.g. via this.storage.getScanningPolicyData()) and honor it: if a persisted policy exists, use that rather than overwriting with this.scanPolicy; only write back to storage when an explicit, intentional policy change occurred (for example when this.scanPolicy is non-null and different from the stored value or when enableSingleAddressPolicy() triggers an explicit update). Update the logic in start() to load the stored policy and conditionally call setScanningPolicyData only when you intend to change the persisted value.
179-200:⚠️ Potential issue | 🟠 MajorMake
scanPolicyoptional in the constructor type.The default
scanPolicy = nullalready supports omission at runtime, but the type annotation still marks the property as mandatory, breaking the API contract for existing callers.✏️ Minimal fix
- scanPolicy: AddressScanPolicyData | null; + scanPolicy?: AddressScanPolicyData | null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.ts` around lines 179 - 200, The constructor's typed parameter list marks scanPolicy as required even though the runtime default is scanPolicy = null; update the type for the constructor parameters to make scanPolicy optional (e.g., change scanPolicy: AddressScanPolicyData | null to scanPolicy?: AddressScanPolicyData | null) so callers may omit it; locate the constructor in src/wallet/wallet.ts and adjust the parameter type in the object type annotation 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__/storage/common_store.test.ts`:
- Around line 267-272: The test is passing an object with excess properties to
MemoryStore.saveAddress: the literal given to saveAddress (used with
IAddressInfo) includes numTransactions and transactions which are not part of
IAddressInfo (numTransactions belongs to IAddressMetadata and transactions is
undefined); fix the test by providing only the IAddressInfo fields to
saveAddress (e.g., base58 and bip32AddressIndex) and, if you need
numTransactions or transactions, pass them via the appropriate metadata API or
attach an IAddressMetadata object where required (reference saveAddress,
MemoryStore.saveAddress, IAddressInfo, IAddressMetadata).
In `@src/storage/memory_store.ts`:
- Around line 344-350: When SCANNING_POLICY.SINGLE_ADDRESS is enabled, the
stored current address index must be clamped to 0 so getCurrentAddress() won't
keep returning a non-first address; update setCurrentAddressIndex(index: number)
to detect when this.walletData.scanPolicyData?.policy ===
SCANNING_POLICY.SINGLE_ADDRESS and if so write the stored index as 0 (instead of
returning early or leaving a >0 value), ensuring the persisted
walletData.currentAddressIndex (or equivalent field used by getCurrentAddress)
is reset and saved so the change takes effect immediately.
---
Outside diff comments:
In `@src/wallet/wallet.ts`:
- Around line 410-420: The start() method is overwriting persisted scanning
policy by unconditionally calling
this.storage.setScanningPolicyData(this.scanPolicy); instead, read the stored
policy first (e.g. via this.storage.getScanningPolicyData()) and honor it: if a
persisted policy exists, use that rather than overwriting with this.scanPolicy;
only write back to storage when an explicit, intentional policy change occurred
(for example when this.scanPolicy is non-null and different from the stored
value or when enableSingleAddressPolicy() triggers an explicit update). Update
the logic in start() to load the stored policy and conditionally call
setScanningPolicyData only when you intend to change the persisted value.
- Around line 179-200: The constructor's typed parameter list marks scanPolicy
as required even though the runtime default is scanPolicy = null; update the
type for the constructor parameters to make scanPolicy optional (e.g., change
scanPolicy: AddressScanPolicyData | null to scanPolicy?: AddressScanPolicyData |
null) so callers may omit it; locate the constructor in src/wallet/wallet.ts and
adjust the parameter type in the object type annotation accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bac6d56a-a2cf-46cc-9ae2-b73e5565f94b
📒 Files selected for processing (8)
__tests__/new/singleAddressPolicy.test.ts__tests__/storage/common_store.test.ts__tests__/types.test.ts__tests__/utils/storage.test.tssrc/storage/memory_store.tssrc/types.tssrc/utils/storage.tssrc/wallet/wallet.ts
Comment on lines
+267
to
+272
| await store.saveAddress({ | ||
| base58: 'addr0', | ||
| bip32AddressIndex: 0, | ||
| numTransactions: 0, | ||
| transactions: [], | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show the saveAddress contract and the fixture passed to it.
rg -n -C2 'interface IAddressInfo|async saveAddress\(info: IAddressInfo\)' src/types.ts src/storage/memory_store.ts
rg -n -C2 'saveAddress\(\{|numTransactions: 0|transactions: \[\]' __tests__/storage/common_store.test.tsRepository: HathorNetwork/hathor-wallet-lib
Length of output: 786
🏁 Script executed:
sed -n '99,110p' src/types.tsRepository: HathorNetwork/hathor-wallet-lib
Length of output: 389
🏁 Script executed:
rg -n 'transactions' src/types.ts -A2 -B2Repository: HathorNetwork/hathor-wallet-lib
Length of output: 509
🏁 Script executed:
rg -n 'IAddressMetadata|transactions:' src/types.ts -B2 -A4Repository: HathorNetwork/hathor-wallet-lib
Length of output: 1554
🏁 Script executed:
rg -n 'transactions' __tests__/storage/common_store.test.ts -B5 -A5Repository: HathorNetwork/hathor-wallet-lib
Length of output: 451
saveAddress() fixture has excess properties.
MemoryStore.saveAddress() accepts IAddressInfo, but numTransactions and transactions do not belong to that interface. numTransactions is part of IAddressMetadata, and transactions is not defined in any address-related interface. This object literal will fail TypeScript excess-property checks.
🧩 Minimal fix
await store.saveAddress({
base58: 'addr0',
bip32AddressIndex: 0,
- numTransactions: 0,
- transactions: [],
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await store.saveAddress({ | |
| base58: 'addr0', | |
| bip32AddressIndex: 0, | |
| numTransactions: 0, | |
| transactions: [], | |
| }); | |
| await store.saveAddress({ | |
| base58: 'addr0', | |
| bip32AddressIndex: 0, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/storage/common_store.test.ts` around lines 267 - 272, The test is
passing an object with excess properties to MemoryStore.saveAddress: the literal
given to saveAddress (used with IAddressInfo) includes numTransactions and
transactions which are not part of IAddressInfo (numTransactions belongs to
IAddressMetadata and transactions is undefined); fix the test by providing only
the IAddressInfo fields to saveAddress (e.g., base58 and bip32AddressIndex) and,
if you need numTransactions or transactions, pass them via the appropriate
metadata API or attach an IAddressMetadata object where required (reference
saveAddress, MemoryStore.saveAddress, IAddressInfo, IAddressMetadata).
Contributor
There was a problem hiding this comment.
@r4mmer It's worth noting that this test will only work while we don't activate TypeScript validation for test files.
It would be nice to make it fully compliant from the get-go, but it's not mandatory right now.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Comment on lines
344
to
+350
| async setCurrentAddressIndex(index: number): Promise<void> { | ||
| if ( | ||
| this.walletData.scanPolicyData?.policy === SCANNING_POLICY.SINGLE_ADDRESS && | ||
| index > 0 | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Normalize currentAddressIndex when single-address mode is enabled.
This guard only blocks future increments. If the wallet already had currentAddressIndex > 0 when the policy flips, getCurrentAddress() keeps serving that non-first address until some other path resets it. Please clamp/reset the stored index as part of enabling the policy.
🔧 Suggested fix
async setScanningPolicyData(data: AddressScanPolicyData): Promise<void> {
this.walletData.scanPolicyData = data;
+ if (data.policy === SCANNING_POLICY.SINGLE_ADDRESS) {
+ this.walletData.currentAddressIndex =
+ this.walletData.currentAddressIndex === -1 ? -1 : 0;
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/storage/memory_store.ts` around lines 344 - 350, When
SCANNING_POLICY.SINGLE_ADDRESS is enabled, the stored current address index must
be clamped to 0 so getCurrentAddress() won't keep returning a non-first address;
update setCurrentAddressIndex(index: number) to detect when
this.walletData.scanPolicyData?.policy === SCANNING_POLICY.SINGLE_ADDRESS and if
so write the stored index as 0 (instead of returning early or leaving a >0
value), ensuring the persisted walletData.currentAddressIndex (or equivalent
field used by getCurrentAddress) is reset and saved so the change takes effect
immediately.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/wallet/wallet.ts (3)
1562-1565: Same defensive check consideration asgetCurrentAddress.
getNextAddressshares the same potential issue whennewAddressesis empty in single-address mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.ts` around lines 1562 - 1565, getNextAddress currently assumes newAddresses[0] exists when singleAddress is true, mirroring the same missing defensive check in getCurrentAddress; update getNextAddress to validate that this.newAddresses is non-empty (and this.newAddresses[0] is defined) before returning it, and otherwise throw or handle the error consistently with getCurrentAddress (e.g., throw a descriptive error like "No addresses available" or return a fallback), ensuring you reference the getNextAddress method, the singleAddress flag, and the newAddresses array when making the change.
1579-1606: Track the TODO for wallet-service API integration.Line 1588 indicates incomplete implementation. Consider opening an issue to track the wallet-service API call for setting single-address mode on the backend.
Would you like me to open an issue to track this follow-up work?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.ts` around lines 1579 - 1606, The enableSingleAddressPolicy method contains a TODO to call the wallet-service API to persist the single-address mode; add the missing backend integration by invoking the appropriate wallet-service endpoint (e.g., a setScanningPolicy or setSingleAddress API) before switching local storage, handle and surface API errors (throw WalletError or log via process logger) and ensure idempotency so enableSingleAddressPolicy updates both remote state and local state (references: enableSingleAddressPolicy, storage.setScanningPolicyData, hasTxOutsideFirstAddress, getAddressAtIndex, getAddressPathForIndex); if the API is not yet implemented, open a tracked issue referencing this method and the required wallet-service endpoint so the backend work is scheduled.
1524-1528: Consider defensive check for emptynewAddresses.If
getCurrentAddressis called before addresses are loaded (e.g., during startup edge cases), accessingthis.newAddresses[0]could fail.💡 Optional defensive pattern
getCurrentAddress({ markAsUsed = false } = {}): AddressInfoObject { if (this.singleAddress) { + if (!this.newAddresses.length) { + throw new WalletError('Addresses not yet loaded'); + } return this.newAddresses[0]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.ts` around lines 1524 - 1528, getCurrentAddress currently assumes this.newAddresses[0] exists when this.singleAddress is true; add a defensive check in getCurrentAddress to handle an empty newAddresses array (e.g., if (!this.newAddresses || this.newAddresses.length === 0) ) and either throw a descriptive Error (e.g., "No addresses loaded yet in getCurrentAddress") or return a safe value (null/undefined) consistent with callers; update getCurrentAddress (and any callers of getCurrentAddress) to handle the chosen behavior so startup edge-cases don't cause runtime exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wallet/wallet.ts`:
- Around line 420-422: this.scanPolicy may be null and is being passed directly
to storage.setScanningPolicyData after calling
this.setState(walletState.LOADING); add a null check to avoid persisting invalid
data: before calling this.storage.setScanningPolicyData(this.scanPolicy) ensure
this.scanPolicy is not null/undefined (e.g., if (!this.scanPolicy) skip
persisting or call storage.clearScanningPolicyData()), so modify the block
around setState, setScanningPolicyData and reference the scanPolicy property and
storage.setScanningPolicyData method to conditionally persist only when a valid
policy exists.
---
Nitpick comments:
In `@src/wallet/wallet.ts`:
- Around line 1562-1565: getNextAddress currently assumes newAddresses[0] exists
when singleAddress is true, mirroring the same missing defensive check in
getCurrentAddress; update getNextAddress to validate that this.newAddresses is
non-empty (and this.newAddresses[0] is defined) before returning it, and
otherwise throw or handle the error consistently with getCurrentAddress (e.g.,
throw a descriptive error like "No addresses available" or return a fallback),
ensuring you reference the getNextAddress method, the singleAddress flag, and
the newAddresses array when making the change.
- Around line 1579-1606: The enableSingleAddressPolicy method contains a TODO to
call the wallet-service API to persist the single-address mode; add the missing
backend integration by invoking the appropriate wallet-service endpoint (e.g., a
setScanningPolicy or setSingleAddress API) before switching local storage,
handle and surface API errors (throw WalletError or log via process logger) and
ensure idempotency so enableSingleAddressPolicy updates both remote state and
local state (references: enableSingleAddressPolicy,
storage.setScanningPolicyData, hasTxOutsideFirstAddress, getAddressAtIndex,
getAddressPathForIndex); if the API is not yet implemented, open a tracked issue
referencing this method and the required wallet-service endpoint so the backend
work is scheduled.
- Around line 1524-1528: getCurrentAddress currently assumes
this.newAddresses[0] exists when this.singleAddress is true; add a defensive
check in getCurrentAddress to handle an empty newAddresses array (e.g., if
(!this.newAddresses || this.newAddresses.length === 0) ) and either throw a
descriptive Error (e.g., "No addresses loaded yet in getCurrentAddress") or
return a safe value (null/undefined) consistent with callers; update
getCurrentAddress (and any callers of getCurrentAddress) to handle the chosen
behavior so startup edge-cases don't cause runtime exceptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be7e09fd-41d2-4627-9585-6bbea4f69eb6
📒 Files selected for processing (6)
__tests__/new/singleAddressPolicy.test.ts__tests__/storage/common_store.test.ts__tests__/utils/storage.test.tssrc/storage/memory_store.tssrc/types.tssrc/wallet/wallet.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/utils/storage.test.ts
- src/types.ts
- tests/storage/common_store.test.ts
r4mmer
force-pushed
the
feat/single-address-policy-2
branch
from
4e32ed2 to
e673ced
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/walletservice_facade.test.ts`:
- Around line 160-210: The new fixture objects singleAddressWallet1 and
singleAddressWallet2 have Prettier/formatting violations; run Prettier (or your
project's formatter) on this test file and reformat the block containing the
singleAddressWallet1 and singleAddressWallet2 definitions so they conform to the
repo style (fix indentation, trailing commas, string quoting and array
formatting), then stage the updated file and push the formatted changes.
- Around line 2428-2434: The assertion checks for the wrong fixture:
nextAddress.address is compared to singleAddressWallet1 but this test uses
singleAddressWallet2; update the expectation in the getNextAddress() assertion
to use singleAddressWallet2.addresses[0] so both current and next address
assertions reference the correct fixture (symbols: wallet, getCurrentAddress,
getNextAddress, singleAddressWallet2).
In `@__tests__/wallet/enableSingleAddressMode.test.ts`:
- Around line 36-42: The tests currently mock walletApi.getAddresses but
enableSingleAddressMode() actually calls
walletApi.getHasTxOutsideFirstAddress(), leaving the real API invoked; update
the mocks in the test to spyOn and mockResolvedValueFor
walletApi.getHasTxOutsideFirstAddress() (returning true/false or the expected
shape) wherever getAddresses was mocked (both the block around the first mock
and the later one around lines referenced in the comment) so the
enableSingleAddressMode() path is exercised with the intended simulated
responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3dca23c-4c97-470e-88ff-f35c58ec7646
📒 Files selected for processing (3)
__tests__/integration/walletservice_facade.test.ts__tests__/wallet/enableSingleAddressMode.test.tssrc/wallet/wallet.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/wallet/wallet.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
__tests__/integration/walletservice_facade.test.ts (1)
2421-2436:⚠️ Potential issue | 🟠 MajorUse the correct fixture in the
getNextAddress()expectation.Line 2435 asserts against
singleAddressWallet1, but this test initializessingleAddressWallet2. The expectation is inconsistent with the test setup.Suggested fix
- expect(nextAddress.address).toBe(singleAddressWallet1.addresses[0]); + expect(nextAddress.address).toBe(singleAddressWallet2.addresses[0]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/walletservice_facade.test.ts` around lines 2421 - 2436, The test uses the wrong fixture in the getNextAddress() assertion: after creating the wallet with buildWalletInstance({ words: singleAddressWallet2.words }) and enabling single-address mode via wallet.enableSingleAddressMode(), update the expectation that compares nextAddress.address to use singleAddressWallet2.addresses[0] (not singleAddressWallet1); locate the assertions around wallet.getCurrentAddress() and wallet.getNextAddress() and change the referenced fixture accordingly.
🧹 Nitpick comments (2)
__tests__/wallet/enableSingleAddressMode.test.ts (1)
71-114: Reduce private-state coupling in address getter tests.Both tests mutate several internals via
(wallet as any)and duplicate setup. Consider extracting a small helper (or enabling mode through public API with mocks) to keep tests resilient to internal refactors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/wallet/enableSingleAddressMode.test.ts` around lines 71 - 114, The tests mutate many private internals of the wallet (state, singleAddress, firstAddress, newAddresses, indexToUse) while testing getCurrentAddress and getNextAddress, making them brittle; refactor by extracting a small test helper (e.g., setupSingleAddressWallet or enableSingleAddressModeMock) that centralizes setting the wallet into Ready single-address mode and initializes firstAddress/newAddresses/indexToUse, or alternatively drive the mode through the public API with a mocked dependency; update both tests to call that helper so they no longer directly cast to any and duplicate the same setup logic for getCurrentAddress and getNextAddress.__tests__/integration/walletservice_facade.test.ts (1)
2449-2470: Consider asserting persisted scanning policy in constructor-mode test.This test verifies address behavior, but adding a
getScanningPolicyData()assertion would better guarantee constructor-started single-address mode is wired to policy persistence too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/walletservice_facade.test.ts` around lines 2449 - 2470, Add an assertion to this constructor-mode test to verify the scanning policy was persisted: after wallet.start({ pinCode, password }), call wallet.getScanningPolicyData() (or the public method that returns scanning policy) and assert it reflects single-address mode (e.g., a policy flag or mode value indicating singleAddressMode and/or that index 0 is set); update the test that constructs HathorWalletServiceWallet with singleAddressMode: true to include this check so the persistence of the scanning policy is validated alongside getCurrentAddress()/getNextAddress().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@__tests__/integration/walletservice_facade.test.ts`:
- Around line 2421-2436: The test uses the wrong fixture in the getNextAddress()
assertion: after creating the wallet with buildWalletInstance({ words:
singleAddressWallet2.words }) and enabling single-address mode via
wallet.enableSingleAddressMode(), update the expectation that compares
nextAddress.address to use singleAddressWallet2.addresses[0] (not
singleAddressWallet1); locate the assertions around wallet.getCurrentAddress()
and wallet.getNextAddress() and change the referenced fixture accordingly.
---
Nitpick comments:
In `@__tests__/integration/walletservice_facade.test.ts`:
- Around line 2449-2470: Add an assertion to this constructor-mode test to
verify the scanning policy was persisted: after wallet.start({ pinCode, password
}), call wallet.getScanningPolicyData() (or the public method that returns
scanning policy) and assert it reflects single-address mode (e.g., a policy flag
or mode value indicating singleAddressMode and/or that index 0 is set); update
the test that constructs HathorWalletServiceWallet with singleAddressMode: true
to include this check so the persistence of the scanning policy is validated
alongside getCurrentAddress()/getNextAddress().
In `@__tests__/wallet/enableSingleAddressMode.test.ts`:
- Around line 71-114: The tests mutate many private internals of the wallet
(state, singleAddress, firstAddress, newAddresses, indexToUse) while testing
getCurrentAddress and getNextAddress, making them brittle; refactor by
extracting a small test helper (e.g., setupSingleAddressWallet or
enableSingleAddressModeMock) that centralizes setting the wallet into Ready
single-address mode and initializes firstAddress/newAddresses/indexToUse, or
alternatively drive the mode through the public API with a mocked dependency;
update both tests to call that helper so they no longer directly cast to any and
duplicate the same setup logic for getCurrentAddress and getNextAddress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93572ae7-2ab3-4655-b87f-6edf958c38c2
📒 Files selected for processing (3)
__tests__/integration/walletservice_facade.test.ts__tests__/wallet/enableSingleAddressMode.test.tssrc/wallet/wallet.ts
✅ Files skipped from review due to trivial changes (1)
- src/wallet/wallet.ts
| private async onWalletReady(skipAddressFetch: boolean = false) { | ||
| // We should wait for new addresses before setting wallet to ready | ||
| // Skip address fetching if requested (useful for read-only wallets that don't need gap addresses) | ||
| if (!skipAddressFetch) { |
Member
There was a problem hiding this comment.
We should skip also if it's singleAddress mode
Member
Author
There was a problem hiding this comment.
Not really because we have a safe-guard for single addresses on getNewAddresses.
Since I use the newAddresses array I had to protect against changes where it is updated, this way any other method or new tx that arrives will just be resetting the first address instead of calling the gap-limit addresses.
Member
|
I think it's also missing the |
r4mmer
force-pushed
the
feat/single-address-policy-2
branch
from
27b7069 to
dd5fba1
Compare
tuliomir
previously approved these changes
Apr 2, 2026
pedroferreira1
previously approved these changes
Apr 6, 2026
pedroferreira1
approved these changes
Apr 6, 2026
tuliomir
approved these changes
Apr 6, 2026
github-project-automation
bot
moved this from In Review (Done)
to Waiting to be deployed
in Hathor Network
This was referenced Apr 7, 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.
Acceptance Criteria
Security Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests