refact: move fee test suite to the right folder#1046
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:
📝 WalkthroughWalkthroughAdds a new end-to-end integration test suite covering FeeBlueprint nano-contract flows and refactors an existing template transaction fee test into a template-driven style; introduces a shared transaction validation helper and restores test lifecycle setup/teardown. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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 #1046 +/- ##
=======================================
Coverage 87.86% 87.86%
=======================================
Files 114 114
Lines 8828 8828
Branches 2006 2006
=======================================
Hits 7757 7757
Misses 1043 1043
Partials 28 28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
__tests__/integration/nanocontracts/fee.test.ts (2)
137-167: Add a regression case for missing nano tx fields.These negative tests cover fee handling, but the wallet entry points exercised throughout this suite still have the known runtime-validation hole for
blueprintId,ncId, andargs. Please add at least one rejected-promise case for missing required fields so this suite fails when malformed nano tx data is accepted again.Based on learnings In
src/new/wallet.ts,createNanoContractTransactionandcreateNanoContractCreateTokenTransactionnon-null assert optionalCreateNanoTxDatafields without prior runtime validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/nanocontracts/fee.test.ts` around lines 137 - 167, Add a regression test that calls the wallet nano-tx entrypoint with missing required CreateNanoTxData fields and asserts the promise is rejected; specifically, invoke hWallet.createAndSendNanoContractCreateTokenTransaction (and/or hWallet.createAndSendNanoContractTransaction) with malformed data that omits blueprintId, ncId, or args and use expect(...).rejects.toThrow(...) to assert a runtime validation error is raised. Place the test alongside the existing fee tests and reuse address from hWallet.getAddressAtIndex(0); ensure the assertion targets an error message indicating the missing field so the suite fails if src/new/wallet.ts (createNanoContractTransaction/createNanoContractCreateTokenTransaction) again accepts null/undefined fields.
172-216: Avoid coupling this assertion to coin-selection internals.
getBalance()is the aggregate wallet balance, butoutputs[3]is only the change from whichever HTR inputs were selected for this tx. This stays correct only while the wallet happens to spend a single “whole-balance” UTXO. Prefer asserting the wallet balance delta after confirmation, or the contract/header deltas, instead of the exact change-output amount.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/nanocontracts/fee.test.ts` around lines 172 - 216, The test couples to coin-selection by asserting createTokenTx.outputs[3].value equals a computed change; instead, verify balance delta or transaction effects independent of specific change UTXO. Replace the direct output check on createTokenTx.outputs[3] with: capture pre- and post-confirmation wallet balances via hWallet.getBalance(), call checkTxValid(...) to confirm the tx, then assert the unlocked balance decreased by the deposit+fee (10n + 1n) or assert expected token outputs/authorities on createTokenTx (outputs[0..2]) and that the wallet HTR balance delta equals -11n; reference hWallet.getBalance, hWallet.createAndSendNanoContractCreateTokenTransaction, checkTxValid, and createTokenTx when making the change.
🤖 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/nanocontracts/fee.test.ts`:
- Around line 36-44: The helper checkTxValid should guard the response of
wallet.getFullTxById(txId) before accessing txAfterExecution.meta: after calling
getFullTxById in checkTxValid, assert that the fetch succeeded (or that
txAfterExecution is defined and has a success flag) and if not, fail with a
clear message containing txId and the raw response; only then assert on
txAfterExecution.meta. Update the function checkTxValid and references to
getFullTxById/txAfterExecution to first check success/definedness and include
the response in the failure message to avoid crashes on transient lookup
failures.
In `@__tests__/integration/template/transaction/fee.test.ts`:
- Around line 49-50: The test currently only awaits block inclusion via
waitTxConfirmed for setup txs (e.g., initTx) but doesn’t assert they weren’t
voided; after waitForTxReceived/ waitTxConfirmed add the same non-voided
validation already implemented in checkTxValid by calling checkTxValid(hWallet,
initTx.hash) (or the equivalent helper) to fail the test if meta.voided_by is
set; apply the same change for the other setup transactions referenced (the
calls at the other noted locations) so each
setup/init/token-creation/setup-withdrawal is explicitly validated as non-voided
before proceeding.
- Around line 116-124: The helper checkTxValid dereferences
txAfterExecution.meta unconditionally which can cause a property-access crash on
API failures; update checkTxValid to guard the result of
wallet.getFullTxById(txId) (the txAfterExecution value returned by
getFullTxById) before reading meta — assert/expect txAfterExecution is defined
and has a meta property (or handle the error variant returned by getFullTxById)
and fail the test with a clear message if not, then continue to check
isEmpty(txAfterExecution.meta.voided_by) and meta.first_block only when the
guard passes.
---
Nitpick comments:
In `@__tests__/integration/nanocontracts/fee.test.ts`:
- Around line 137-167: Add a regression test that calls the wallet nano-tx
entrypoint with missing required CreateNanoTxData fields and asserts the promise
is rejected; specifically, invoke
hWallet.createAndSendNanoContractCreateTokenTransaction (and/or
hWallet.createAndSendNanoContractTransaction) with malformed data that omits
blueprintId, ncId, or args and use expect(...).rejects.toThrow(...) to assert a
runtime validation error is raised. Place the test alongside the existing fee
tests and reuse address from hWallet.getAddressAtIndex(0); ensure the assertion
targets an error message indicating the missing field so the suite fails if
src/new/wallet.ts
(createNanoContractTransaction/createNanoContractCreateTokenTransaction) again
accepts null/undefined fields.
- Around line 172-216: The test couples to coin-selection by asserting
createTokenTx.outputs[3].value equals a computed change; instead, verify balance
delta or transaction effects independent of specific change UTXO. Replace the
direct output check on createTokenTx.outputs[3] with: capture pre- and
post-confirmation wallet balances via hWallet.getBalance(), call
checkTxValid(...) to confirm the tx, then assert the unlocked balance decreased
by the deposit+fee (10n + 1n) or assert expected token outputs/authorities on
createTokenTx (outputs[0..2]) and that the wallet HTR balance delta equals -11n;
reference hWallet.getBalance,
hWallet.createAndSendNanoContractCreateTokenTransaction, checkTxValid, and
createTokenTx when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2b1fe90-14b4-40bb-a499-4de261332287
📒 Files selected for processing (2)
__tests__/integration/nanocontracts/fee.test.ts__tests__/integration/template/transaction/fee.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/integration/template/transaction/fee.test.ts (1)
132-144: Consider extractingcheckTxValidto a shared test helper.This helper is duplicated in
__tests__/integration/nanocontracts/fee.test.ts. Extracting it to a shared utility (e.g., in__tests__/integration/helpers/) would reduce duplication and centralize transaction validation logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/template/transaction/fee.test.ts` around lines 132 - 144, The duplicated checkTxValid function should be moved to a shared test helper: create a new helper file (e.g., tests integration helper) that exports checkTxValid (preserving signature checkTxValid(wallet, tx)), import and use that helper from both __tests__/integration/template/transaction/fee.test.ts and __tests__/integration/nanocontracts/fee.test.ts, remove the local copies in each test file, and update any relative import paths and exports so waitForTxReceived, waitTxConfirmed, isEmpty usages remain resolved (or re-export/require them from the helper) to keep behavior identical.
🤖 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/template/transaction/fee.test.ts`:
- Around line 132-144: The duplicated checkTxValid function should be moved to a
shared test helper: create a new helper file (e.g., tests integration helper)
that exports checkTxValid (preserving signature checkTxValid(wallet, tx)),
import and use that helper from both
__tests__/integration/template/transaction/fee.test.ts and
__tests__/integration/nanocontracts/fee.test.ts, remove the local copies in each
test file, and update any relative import paths and exports so
waitForTxReceived, waitTxConfirmed, isEmpty usages remain resolved (or
re-export/require them from the helper) to keep behavior identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1f42ee4-e9c9-4641-8801-1b2305d671ce
📒 Files selected for processing (2)
__tests__/integration/nanocontracts/fee.test.ts__tests__/integration/template/transaction/fee.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/integration/nanocontracts/fee.test.ts
dd7c88e to
11fb9e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
__tests__/integration/nanocontracts/fee.test.ts (2)
900-948: Remove unnecessary non-null assertion ontx.At lines 941 and 946,
tx!is used buttxis already assigned from theawaitresult. The!is unnecessary since the code would fail earlier iftxwere null.✨ Suggested cleanup
// Verify FeeHeader exists and has correct fee - const feeHeader = tx!.getFeeHeader(); + const feeHeader = tx.getFeeHeader(); expect(feeHeader).not.toBeNull(); expect(feeHeader!.entries[0].amount).toBe(1n); // Verify token was created with FEE version - const tokenDetails = await hWallet.getTokenDetails(tx!.hash!); + const tokenDetails = await hWallet.getTokenDetails(tx.hash!); expect(tokenDetails.tokenInfo.version).toBe(TokenVersion.FEE);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/nanocontracts/fee.test.ts` around lines 900 - 948, The test uses unnecessary non-null assertions on tx (tx!) when calling getFeeHeader() and getTokenDetails(tx!.hash!), but tx is already assigned from the awaited result and earlier assertions ensure it exists; remove the redundant `!` operators. Update the two calls to use tx.getFeeHeader() and hWallet.getTokenDetails(tx.hash) (referenced symbols: tx, getFeeHeader, hash, hWallet.getTokenDetails, CreateTokenTransaction) so the code relies on normal type flow rather than non-null assertions.
950-1018: Same cleanup applies here at line 1016.
tx!.hash!→tx.hash!for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/nanocontracts/fee.test.ts` around lines 950 - 1018, The test uses the non-null assertion twice on tx (`tx!.hash!`) but should use the local variable `tx` without the redundant assertion for consistency; update the tokenDetails call to use `tx.hash!` (referencing the `tx` variable and the `getTokenDetails` usage in this test case where `CreateTokenTransaction` is asserted) so only the hash uses a single non-null assertion.__tests__/integration/template/transaction/fee.test.ts (1)
132-144: Consider extractingcheckTxValidto a shared test utility.This helper is duplicated identically in
__tests__/integration/nanocontracts/fee.test.ts(lines 36-48). Since both files are part of this PR, consider extracting it to a shared location like__tests__/integration/helpers/transaction.helper.tsto avoid duplication.The implementation itself is correct—it properly guards the
getFullTxById()response before accessingmeta.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/template/transaction/fee.test.ts` around lines 132 - 144, The helper function checkTxValid is duplicated; extract it into a shared test utility (e.g., create __tests__/integration/helpers/transaction.helper.ts) that exports checkTxValid, keeping the same internals which reference waitForTxReceived, waitTxConfirmed, and wallet.getFullTxById; then import checkTxValid from that helper in both fee.test.ts files and remove the duplicated local definitions so both tests use the single exported function.
🤖 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/nanocontracts/fee.test.ts`:
- Around line 603-624: The test creates an emptyWallet via generateWalletHelper
and never stops it; wrap the usage of emptyWallet (the call to
emptyWallet.createAndSendNanoContractTransaction/expect) in a try/finally (or
ensure cleanup with afterEach) and call await emptyWallet.stop() in the finally
so the wallet is always stopped even when the expectation throws; reference
emptyWallet, generateWalletHelper, and createAndSendNanoContractTransaction to
locate where to add the cleanup.
---
Nitpick comments:
In `@__tests__/integration/nanocontracts/fee.test.ts`:
- Around line 900-948: The test uses unnecessary non-null assertions on tx (tx!)
when calling getFeeHeader() and getTokenDetails(tx!.hash!), but tx is already
assigned from the awaited result and earlier assertions ensure it exists; remove
the redundant `!` operators. Update the two calls to use tx.getFeeHeader() and
hWallet.getTokenDetails(tx.hash) (referenced symbols: tx, getFeeHeader, hash,
hWallet.getTokenDetails, CreateTokenTransaction) so the code relies on normal
type flow rather than non-null assertions.
- Around line 950-1018: The test uses the non-null assertion twice on tx
(`tx!.hash!`) but should use the local variable `tx` without the redundant
assertion for consistency; update the tokenDetails call to use `tx.hash!`
(referencing the `tx` variable and the `getTokenDetails` usage in this test case
where `CreateTokenTransaction` is asserted) so only the hash uses a single
non-null assertion.
In `@__tests__/integration/template/transaction/fee.test.ts`:
- Around line 132-144: The helper function checkTxValid is duplicated; extract
it into a shared test utility (e.g., create
__tests__/integration/helpers/transaction.helper.ts) that exports checkTxValid,
keeping the same internals which reference waitForTxReceived, waitTxConfirmed,
and wallet.getFullTxById; then import checkTxValid from that helper in both
fee.test.ts files and remove the duplicated local definitions so both tests use
the single exported function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58aa7f76-46ac-418a-ad1e-c58076b30ba8
📒 Files selected for processing (2)
__tests__/integration/nanocontracts/fee.test.ts__tests__/integration/template/transaction/fee.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
__tests__/integration/template/transaction/fee.test.ts (2)
49-54: Deduplicate setup tx validation to one helper to keep checks consistent.The same receipt/confirmation/fetch/voided validation is repeated four times. Reuse a single helper (or reuse
checkTxValid) so setup assertions don’t diverge over time.♻️ Suggested refactor
+ const checkSetupTxValid = async (wallet: HathorWallet, tx: { hash?: string }, label: string) => { + const txId = tx.hash; + expect(txId).toBeDefined(); + await waitForTxReceived(wallet, txId); + await waitTxConfirmed(wallet, txId, null); + const txData = await wallet.getFullTxById(txId); + if (!txData.success || !isEmpty(txData.meta.voided_by)) { + throw new Error(`Setup failed: ${label} ${txId} was voided or failed to fetch`); + } + }; ... - await waitForTxReceived(hWallet, initTx.hash); - await waitTxConfirmed(hWallet, initTx.hash, null); - const initTxData = await hWallet.getFullTxById(initTx.hash); - if (!initTxData.success || !isEmpty(initTxData.meta.voided_by)) { - throw new Error(`Setup failed: initTx ${initTx.hash} was voided or failed to fetch`); - } + await checkSetupTxValid(hWallet, initTx, 'initTx'); ... - await waitForTxReceived(hWallet, dbtTx.hash); - await waitTxConfirmed(hWallet, dbtTx.hash, null); - const dbtTxData = await hWallet.getFullTxById(dbtTx.hash); - if (!dbtTxData.success || !isEmpty(dbtTxData.meta.voided_by)) { - throw new Error(`Setup failed: dbtTx ${dbtTx.hash} was voided or failed to fetch`); - } + await checkSetupTxValid(hWallet, dbtTx, 'dbtTx');Also applies to: 74-79, 96-101, 118-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/template/transaction/fee.test.ts` around lines 49 - 54, The test repeats the same receipt/confirmation/fetch/voided validation sequence in multiple places; extract and reuse a single helper (or the existing checkTxValid) to ensure consistency. Replace the repeated block using await waitForTxReceived(hWallet, initTx.hash); await waitTxConfirmed(hWallet, initTx.hash, null); const initTxData = await hWallet.getFullTxById(initTx.hash); if (!initTxData.success || !isEmpty(initTxData.meta.voided_by)) { ... } with a call to the new helper (e.g., await assertTxValid(hWallet, initTx.hash) or reuse checkTxValid(hWallet, initTx.hash)), implement that helper to call waitForTxReceived, waitTxConfirmed, fetch via getFullTxById and assert success and no voided_by, and update all occurrences (lines shown and similar blocks) to call this helper.
24-24: UsegenerateWalletHelper()instead ofgenerateWalletHelper(null).Passing
nullfor optional parameters conflicts with the repository's TypeScript migration convention. Useundefined(by omitting the argument) instead, as shown in the helper's own JSDoc examples.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/template/transaction/fee.test.ts` at line 24, Replace the call that passes null to the optional parameter by invoking the helper without an argument: change generateWalletHelper(null) to generateWalletHelper() where hWallet is assigned; this follows the TypeScript migration convention and the helper's JSDoc examples by using undefined/omitting the parameter instead of passing null.
🤖 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/template/transaction/fee.test.ts`:
- Around line 126-129: The afterAll teardown currently calls await
hWallet.stop() unconditionally which can throw if beforeAll failed to assign
hWallet; guard the teardown by checking hWallet exists/is initialized before
calling hWallet.stop() (e.g., if (hWallet) await hWallet.stop()), leaving await
stopAllWallets() and await GenesisWalletHelper.clearListeners() as-is so they
still run even when hWallet was never created; update the afterAll block that
references afterAll and hWallet.stop accordingly.
---
Nitpick comments:
In `@__tests__/integration/template/transaction/fee.test.ts`:
- Around line 49-54: The test repeats the same receipt/confirmation/fetch/voided
validation sequence in multiple places; extract and reuse a single helper (or
the existing checkTxValid) to ensure consistency. Replace the repeated block
using await waitForTxReceived(hWallet, initTx.hash); await
waitTxConfirmed(hWallet, initTx.hash, null); const initTxData = await
hWallet.getFullTxById(initTx.hash); if (!initTxData.success ||
!isEmpty(initTxData.meta.voided_by)) { ... } with a call to the new helper
(e.g., await assertTxValid(hWallet, initTx.hash) or reuse checkTxValid(hWallet,
initTx.hash)), implement that helper to call waitForTxReceived, waitTxConfirmed,
fetch via getFullTxById and assert success and no voided_by, and update all
occurrences (lines shown and similar blocks) to call this helper.
- Line 24: Replace the call that passes null to the optional parameter by
invoking the helper without an argument: change generateWalletHelper(null) to
generateWalletHelper() where hWallet is assigned; this follows the TypeScript
migration convention and the helper's JSDoc examples by using undefined/omitting
the parameter instead of passing null.
🪄 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: d32f7c49-002a-4eb2-80dc-2582aa9581ba
📒 Files selected for processing (1)
__tests__/integration/template/transaction/fee.test.ts
a9de44d to
9e5c17d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/integration/template/transaction/fee.test.ts (1)
20-20: Remove unused variable_dbtUid.The variable
_dbtUidis assigned on line 80 but never read anywhere in the test suite. While the underscore prefix conventionally signals intentional non-use, this is dead code that can be removed to improve clarity.🧹 Proposed cleanup
let contractId: string; let fbtUid: string; - let _dbtUid: string;And remove the assignment:
- const dbtState = await ncApi.getNanoContractState(contractId, ['dbt_uid'], [], []); - _dbtUid = dbtState.fields.dbt_uid.value;Also applies to: 79-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/template/transaction/fee.test.ts` at line 20, Remove the dead variable declaration and assignment for _dbtUid: delete the top-level let _dbtUid: string; declaration and the later assignment to _dbtUid in the test setup (around the block where you assign dbt UIDs), since _dbtUid is never read; ensure no other code references _dbtUid remain (if references exist, either use the value or remove those references).
🤖 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/template/transaction/fee.test.ts`:
- Line 20: Remove the dead variable declaration and assignment for _dbtUid:
delete the top-level let _dbtUid: string; declaration and the later assignment
to _dbtUid in the test setup (around the block where you assign dbt UIDs), since
_dbtUid is never read; ensure no other code references _dbtUid remain (if
references exist, either use the value or remove those references).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ca71208-7340-4717-aa49-65b27461b5da
📒 Files selected for processing (1)
__tests__/integration/template/transaction/fee.test.ts
refact: move fee test suite to the right folder
* 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>
Acceptance Criteria
/template/folder/nano/folderSecurity Checklist
Summary by CodeRabbit