Test: Shared sendTransaction tests for both facades#1053
Open
Test: Shared sendTransaction tests for both facades#1053
Conversation
Extract sendTransaction and sendManyOutputsTransaction coverage into the shared test framework. Adds sendTransaction adapter method that handles pinCode injection for service facade. - shared/send-transaction.test.ts: internal transfer balance invariant, external transfer balance decrease - fullnode-specific/send-transaction.test.ts: address tracking, custom/fee tokens, multisig, sendManyOutputs - service-specific/send-transaction.test.ts: full structure validation, P2SH target, changeAddress verification Co-Authored-By: Claude Opus 4.6 (1M context) <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:
📝 WalkthroughWalkthroughThis PR expands test adapter APIs with transaction-related operations (sendTransaction, getFullTxById, createToken, getUtxos, sendManyOutputsTransaction, getAddressInfo), updates adapter types, and reorganizes/introduces integration tests into shared and fullnode-specific suites for comprehensive sendTransaction scenarios. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1053 +/- ##
=======================================
Coverage 87.93% 87.93%
=======================================
Files 114 114
Lines 8910 8910
Branches 2030 2030
=======================================
Hits 7835 7835
Misses 1047 1047
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: 1
🧹 Nitpick comments (1)
__tests__/integration/adapters/service.adapter.ts (1)
213-216: Use adapter default pin instead of hardcoded constant.This avoids silent drift between adapter defaults and transaction calls.
♻️ Suggested change
const result = await sw.sendTransaction(address, amount, { ...options, - pinCode: SERVICE_PIN, + pinCode: this.defaultPinCode, });🤖 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 213 - 216, The call to sw.sendTransaction is using a hardcoded SERVICE_PIN constant which can drift from the adapter's configured default; replace SERVICE_PIN with the adapter's default pin accessor (e.g., sw.defaultPin or sw.getDefaultPin()/sw.options.pinCode depending on the adapter API) so the call uses the adapter's configured default pin at runtime; update the sendTransaction invocation in the sw.sendTransaction call site to pass that adapter default instead of the SERVICE_PIN constant.
🤖 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/fullnode-specific/send-transaction.test.ts`:
- Around line 215-216: The test is asserting the wrong fee amount for a
fee-token send that creates a change UTXO: update the expectation from 1n to 2n
so the validateFeeAmount(tx.headers, ...) call reflects two fee-token outputs
(recipient + change). Locate the validateFeeAmount invocation in the
send-transaction test (the one immediately before await
waitForTxReceived(hWallet, tx.hash)) and change its expected fee argument to 2n
to match other fee-token send cases.
---
Nitpick comments:
In `@__tests__/integration/adapters/service.adapter.ts`:
- Around line 213-216: The call to sw.sendTransaction is using a hardcoded
SERVICE_PIN constant which can drift from the adapter's configured default;
replace SERVICE_PIN with the adapter's default pin accessor (e.g., sw.defaultPin
or sw.getDefaultPin()/sw.options.pinCode depending on the adapter API) so the
call uses the adapter's configured default pin at runtime; update the
sendTransaction invocation in the sw.sendTransaction call site to pass that
adapter default instead of the SERVICE_PIN constant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59253e41-7564-4ba1-86b1-97e827263443
📒 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/send-transaction.test.ts__tests__/integration/hathorwallet_facade.test.ts__tests__/integration/service-specific/send-transaction.test.ts__tests__/integration/shared/send-transaction.test.ts__tests__/integration/walletservice_facade.test.ts
- Pass pinCode in fullnode adapter sendTransaction - Correct fee expectation from 1n to 2n (2 outputs) - Replace implicit walletWithTxs funding with explicit precalculated wallet setup in getBalance test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
__tests__/integration/walletservice_facade.test.ts (1)
951-971: EnsurewalletTxscleanup runs even on assertion failures.
walletTxs.stop(...)is currently best-effort at the end of the test body. If any step fails earlier, the wallet may leak and affect subsequent integration tests.♻️ Suggested hardening
it('should return balance array for wallet with transactions', async () => { const { wallet: walletTxs, addresses } = buildWalletInstance(); await walletTxs.start({ pinCode, password }); - - // Fund the wallet so it has transaction history - const fundTx = await gWallet.sendTransaction(addresses[0], 10n, { pinCode }); - await pollForTx(gWallet, fundTx.hash!); - await pollForTx(walletTxs, fundTx.hash!); - - const balances = await walletTxs.getBalance(); - - expect(Array.isArray(balances)).toBe(true); - expect(balances.length).toBeGreaterThanOrEqual(1); - - // Should have HTR balance - const htrBalance = balances.find(b => b.token.id === NATIVE_TOKEN_UID); - expect(htrBalance).toBeDefined(); - expect(typeof htrBalance?.balance).toBe('object'); - - await walletTxs.stop({ cleanStorage: true }); + try { + // Fund the wallet so it has transaction history + const fundTx = await gWallet.sendTransaction(addresses[0], 10n, { pinCode }); + await pollForTx(gWallet, fundTx.hash!); + await pollForTx(walletTxs, fundTx.hash!); + + const balances = await walletTxs.getBalance(); + expect(Array.isArray(balances)).toBe(true); + expect(balances.length).toBeGreaterThanOrEqual(1); + + // Should have HTR balance + const htrBalance = balances.find(b => b.token.id === NATIVE_TOKEN_UID); + expect(htrBalance).toBeDefined(); + expect(typeof htrBalance?.balance).toBe('object'); + } finally { + await walletTxs.stop({ cleanStorage: true }); + } });🤖 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 951 - 971, Wrap the test body in a try/finally so walletTxs.stop({ cleanStorage: true }) always runs even if assertions fail; locate the test "should return balance array for wallet with transactions" and ensure all setup/operations (start, sendTransaction, pollForTx, getBalance, assertions) happen inside the try and call walletTxs.stop(...) in the finally block to guarantee cleanup of the WalletService instance.
🤖 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 185-199: The code calls hWallet.sendTransaction which returns
Promise<Transaction | null>, but the current sendTransaction implementation
dereferences result.hash without checking for null; update the sendTransaction
method to guard the result from hWallet.sendTransaction (the local variable
result), and if it's null either throw a descriptive Error or return a failed
SendTransactionResult, otherwise call waitForTxReceived and
waitUntilNextTimestamp with result.hash and return { hash: result.hash }; ensure
you reference hWallet.sendTransaction, the result variable, waitForTxReceived,
and waitUntilNextTimestamp when making the fix.
---
Nitpick comments:
In `@__tests__/integration/walletservice_facade.test.ts`:
- Around line 951-971: Wrap the test body in a try/finally so walletTxs.stop({
cleanStorage: true }) always runs even if assertions fail; locate the test
"should return balance array for wallet with transactions" and ensure all
setup/operations (start, sendTransaction, pollForTx, getBalance, assertions)
happen inside the try and call walletTxs.stop(...) in the finally block to
guarantee cleanup of the WalletService instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26c93d53-626a-4110-8aae-ea7bf5457fe7
📒 Files selected for processing (3)
__tests__/integration/adapters/fullnode.adapter.ts__tests__/integration/fullnode-specific/send-transaction.test.ts__tests__/integration/walletservice_facade.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/integration/fullnode-specific/send-transaction.test.ts
Wrap balance test body in try/finally to guarantee walletTxs.stop() runs on failure. Add hasAssertions() to prevent silent passes when assertions are skipped. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| /** | ||
| * Validates the total fee amount in a list of headers. | ||
| */ | ||
| function validateFeeAmount(headers: unknown[], expectedFee: bigint) { |
Contributor
There was a problem hiding this comment.
headers can be a instante of Header[]
|
|
||
| const adapter = new ServiceWalletTestAdapter(); | ||
|
|
||
| const walletWithTxs = { |
Contributor
There was a problem hiding this comment.
Does make sense to still using this wallet obj?
Contributor
Author
There was a problem hiding this comment.
Absolutely not, thanks for pointing that out. Fixed on baf474f
raul-oliveira
previously approved these changes
Apr 1, 2026
Replace hardcoded wallet seed/addresses with pre-generated wallets via buildWalletInstance(), change headers parameter type from unknown[] to Header[], and add explicit funding for the changeAddress test to remove implicit test ordering dependency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move structure validation, P2SH, and changeAddress tests from service-specific to shared so they run on both facades. This was possible because getFullTxById() is on IHathorWallet and replaces the service-only getUtxoFromId() for address verification. Enhance adapter interface: SendTransactionResult now includes the full Transaction model, and getFullTxById() is available on both adapters. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove unused dateFormatter and loggers imports to fix lint warnings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move custom token, fee token, and manual-input fee token tests from fullnode-specific to shared tests. Extend adapter interface with createToken, getUtxos, and sendManyOutputsTransaction methods so both facades can run these tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add getAddressInfo to the adapter interface, backed by storage.getAddressInfo() in both facades. The wallet service proxy already maps numTransactions from the REST API, making this test shareable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tuliomir
force-pushed
the
test/shared-send-transaction
branch
from
904c450 to
2f7804f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
__tests__/integration/adapters/service.adapter.ts (1)
214-230: Consider using options.pinCode when provided.The
pinCodeis hardcoded toSERVICE_PINand will override any pinCode passed via options. While this may be intentional for test simplicity, it differs from how thecreateTokenmethod handles it (line 258 uses spread after pinCode, allowing override).If the intent is to always use
SERVICE_PINin tests, consider documenting this behavior. Otherwise, for consistency withcreateToken:♻️ Suggested consistency fix
async sendTransaction( wallet: FuzzyWalletType, address: string, amount: bigint, options?: SendTransactionOptions ): Promise<SendTransactionResult> { const sw = this.concrete(wallet); const result = await sw.sendTransaction(address, amount, { - ...options, pinCode: SERVICE_PIN, + ...options, });🤖 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 214 - 230, The sendTransaction implementation always forces pinCode to SERVICE_PIN which overrides any provided options.pinCode; to make it consistent with createToken, change the options merge in sendTransaction (function sendTransaction) so SERVICE_PIN is set first and then spread options (i.e., pinCode: SERVICE_PIN, ...options) so a passed options.pinCode can override, or alternatively document intentional forced use of SERVICE_PIN if that behavior is desired.
🤖 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/shared/send-transaction-tokens.test.ts`:
- Around line 72-74: The test is waiting for the wrong transaction hash:
adapter.waitForTx(externalWallet, txHash) uses the earlier txHash (first send)
instead of the hash produced by the send to externalAddr. Fix by capturing the
sendTransaction result for the external transfer (e.g., txHashExternal or reuse
txHash by assigning the return of adapter.sendTransaction(wallet, externalAddr,
80n, { token: tokenUid })) and pass that hash into
adapter.waitForTx(externalWallet, <that hash>), ensuring externalWallet waits
for the correct transaction.
---
Nitpick comments:
In `@__tests__/integration/adapters/service.adapter.ts`:
- Around line 214-230: The sendTransaction implementation always forces pinCode
to SERVICE_PIN which overrides any provided options.pinCode; to make it
consistent with createToken, change the options merge in sendTransaction
(function sendTransaction) so SERVICE_PIN is set first and then spread options
(i.e., pinCode: SERVICE_PIN, ...options) so a passed options.pinCode can
override, or alternatively document intentional forced use of SERVICE_PIN if
that behavior is desired.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24b5edac-c369-43b7-adff-947e1d71bebc
📒 Files selected for processing (9)
__tests__/integration/adapters/fullnode.adapter.ts__tests__/integration/adapters/service.adapter.ts__tests__/integration/adapters/types.ts__tests__/integration/fullnode-specific/send-transaction.test.ts__tests__/integration/hathorwallet_facade.test.ts__tests__/integration/shared/send-transaction-address-tracking.test.ts__tests__/integration/shared/send-transaction-tokens.test.ts__tests__/integration/shared/send-transaction.test.ts__tests__/integration/walletservice_facade.test.ts
✅ Files skipped from review due to trivial changes (3)
- tests/integration/fullnode-specific/send-transaction.test.ts
- tests/integration/hathorwallet_facade.test.ts
- tests/integration/shared/send-transaction.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/adapters/fullnode.adapter.ts
- tests/integration/adapters/types.ts
- Fix wrong txHash in custom token test (waited for first send instead of external send) - Fix HTR balance assertion (relative check, not absolute, since tests share wallet state) - Replace wallet.getTx() with adapter.getFullTxById() (getTx not implemented on wallet service) - Revert address tracking to fullnode-specific (the WalletServiceStorageProxy only exists during nano contract signing, not on wallet.storage) - Fix pinCode spread order in service adapter Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tuliomir
commented
Apr 2, 2026
| @@ -1110,21 +948,28 @@ describe('balances', () => { | |||
| }); | |||
|
|
|||
| it('should return balance array for wallet with transactions', async () => { | |||
Contributor
Author
There was a problem hiding this comment.
We had to change these unrelated tests because they used to rely on the state left by sendTransactions tests, an anti-pattern we are fixing now.
Add tests for fee token output-count edge cases: - Single output (no change): validates 1 HTR fee - Five outputs (no change): validates 5 HTR fee Also fix assertion using wrong field (token → token_data) in the manual HTR input test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
# Conflicts: # __tests__/integration/adapters/fullnode.adapter.ts
| const feeHeaders = headers.filter(h => h instanceof FeeHeader); | ||
| expect(feeHeaders).toHaveLength(1); | ||
| const totalFee = (feeHeaders[0] as FeeHeader).entries.reduce( | ||
| (sum, entry) => sum + entry.amount, |
Contributor
There was a problem hiding this comment.
this can be a problem since fee header can contain multiple token and the amount can vary.
For example, htr 1n === dbt 100n
Contributor
Author
There was a problem hiding this comment.
Simplified the logic on 4c027f2 , since the scope of this PR wouldn't allow multiple fee tokens to be transacted at once
| /** Creates a funded wallet and an external wallet for receiving. */ | ||
| async function createFundedPair(htrAmount: bigint) { | ||
| const created = await adapter.createWallet(); | ||
| const w = created.wallet; |
Contributor
There was a problem hiding this comment.
Use a better name like wallet insetad of w
| }); | ||
|
|
||
| it('should send custom fee token transactions', async () => { | ||
| // 10n HTR: 1n token deposit, 2n fee per send × 2 sends = 5n spent → 5n remaining |
Contributor
There was a problem hiding this comment.
I missed the token deposit here. the create fee token issue 1n fee and it's not withdrawable.
| expect(tokenOutputs).toHaveLength(1); | ||
| expect(tokenOutputs[0].value).toBe(200n); | ||
|
|
||
| // 10 HTR - 1 deposit - 1 fee = 8 HTR remaining |
Contributor
There was a problem hiding this comment.
same here about deposit
| expect(tokenOutputs).toHaveLength(5); | ||
| tokenOutputs.forEach((o: { value: bigint }) => expect(o.value).toBe(100n)); | ||
|
|
||
| // 10 HTR - 1 deposit - 5 fee = 4 HTR remaining |
Contributor
There was a problem hiding this comment.
Same here about deposit
- Validate single fee entry instead of summing across tokens with different denominations - Rename `w` to `wallet` in createFundedPair - Clarify deposit comments as non-refundable Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| it('should send custom token transactions', async () => { | ||
| // wallet: 10n HTR | ||
| const { wallet, externalWallet } = await createFundedPair(10n); | ||
| // 100n TTS created, 1n HTR deposit deducted |
Contributor
There was a problem hiding this comment.
Non-blocking: I like to always use DBT and FBT to emphasize which token type we're dealing with. What do you think about it?
raul-oliveira
approved these changes
Apr 9, 2026
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
2 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
sendTransactionandsendManyOutputsTransactiontests fromhathorwallet_facade.test.tsandwalletservice_facade.test.tsinto shared and facade-specific filessendTransactionadapter method toIWalletTestAdapter— handles pinCode injection for service facadeshared/send-transaction.test.tswith adapter-driven tests (internal transfer balance invariant, external transfer balance decrease)fullnode-specific/send-transaction.test.tsservice-specific/send-transaction.test.tsAcceptance Criteria
sendTransactionorsendManyOutputsTransactiondescribe blocks remain in the monolithic facade filesTest plan
npm run testpassesnpm run test_integrationpasses (shared tests run against both facades)🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores