tx-history model assertion by ronaldsg20 · Pull Request #448 · rsksmart/2wp-api

ronaldsg20 · 2026-01-30T20:08:22Z

No description provided.

Adding model validation and tests for tx-history

github-actions · 2026-01-30T20:10:10Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

Package

Version

Score

Details

npm/nock

13.5.6

🟢 4.7

Details

Check	Score	Reason
Code-Review	🟢 6	Found 12/18 approved changesets -- score normalized to 6
Dangerous-Workflow	🟢 10	no dangerous workflow patterns detected
Maintained	🟢 7	8 commit(s) and 1 issue activity found in the last 90 days -- score normalized to 7
Token-Permissions	⚠️ 0	detected GitHub workflow tokens with excessive permissions
Binary-Artifacts	🟢 10	no binaries found in the repo
Pinned-Dependencies	🟢 3	dependency not pinned by hash detected -- score normalized to 3
CII-Best-Practices	⚠️ 0	no effort to earn an OpenSSF best practices badge detected
Fuzzing	⚠️ 0	project is not fuzzed
License	🟢 10	license file detected
Branch-Protection	⚠️ -1	internal error: error during branchesHandler.setup: internal error: some github tokens can't read classic branch protection rules: https://github.com/ossf/scorecard-action/blob/main/docs/authentication/fine-grained-auth-token.md
Signed-Releases	⚠️ -1	no releases found
Security-Policy	⚠️ 0	security policy file not detected
Packaging	🟢 10	packaging workflow detected
SAST	⚠️ 0	SAST tool is not run on all commits -- score normalized to 0
Vulnerabilities	⚠️ 0	17 existing vulnerabilities detected

Scanned Files

package-lock.json

Copilot

Pull request overview

This PR adds comprehensive validation and schema assertions to the transaction history API models and controllers. It extends support for additional tokens and networks, adds new optional fields for liquidity provider tracking, implements strict input validation patterns, and includes extensive acceptance tests to verify the validation behavior.

Changes:

Extended TOKENS enum to include ETH, USDT, USDC, DAI, WBTC, BNB, and Fiat
Extended NETWORKS enum to include Ethereum, BNB Smart Chain, and Lightning Network
Added liquidityProviderName and liquidityProviderId optional fields to TxHistory model with validation
Added comprehensive input validation patterns to controller endpoints (address, page, txHash)
Added txHash validation pattern to RegisterPayload model
Added 533 lines of acceptance tests covering positive and negative validation scenarios
Modified test script to run both unit and acceptance tests together
Downgraded nock package from v14 to v13
Fixed import ordering in fireblocks acceptance test
Updated input sanitization test to verify SQL injection prevention in txHash field

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
src/models/tx-history.model.ts	Added new tokens/networks to enums, added liquidityProviderName and liquidityProviderId fields with validation
src/models/register-payload.model.ts	Added regex validation pattern for txHash field to prevent injection attacks
src/controllers/tx-history.controller.ts	Added parameter validation schemas for address, page, and txHash with regex patterns and error responses
src/tests/acceptance/tx-history.acceptance.ts	New comprehensive acceptance test suite with 533 lines testing all validation scenarios
src/tests/acceptance/input-sanitization.acceptance.ts	Updated SQL injection test to expect rejection (422) instead of acceptance, removed skipped hex data test
src/tests/acceptance/fireblocks.controller.acceptance.ts	Reordered imports to follow consistent pattern
package.json	Modified test script to run both unit and acceptance tests; downgraded nock from v14 to v13
package-lock.json	Updated dependency tree reflecting nock downgrade and removed related dependencies

src/controllers/tx-history.controller.ts

src/models/tx-history.model.ts

package.json

src/models/tx-history.model.ts

src/models/register-payload.model.ts

src/models/tx-history.model.ts

package.json

src/__tests__/acceptance/input-sanitization.acceptance.ts

src/models/tx-history.model.ts

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 8 comments.

src/controllers/tx-history.controller.ts

src/models/tx-history.model.ts

src/controllers/tx-history.controller.ts

src/__tests__/acceptance/tx-history.acceptance.ts

src/__tests__/acceptance/input-sanitization.acceptance.ts

src/controllers/tx-history.controller.ts

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.

Copilot · 2026-02-13T19:56:24Z

src/models/tx-history.model.ts

 import {SearchableModel} from './rsk/searchable-model';

-export const SOURCES = ['FLYOVER', 'SWAP', 'POWPEG', 'UNION', 'LIFI', 'SYMBIOSIS'] as const;
+export const SOURCES = ['FLYOVER', 'SWAP', 'POWPEG', 'UNION', 'LIFI', 'SYMBIOSIS', 'teks-staging'] as const;


The value 'teks-staging' added to SOURCES uses lowercase with a hyphen, which is inconsistent with all other values in the array that use uppercase letters (e.g., 'FLYOVER', 'SWAP', 'POWPEG', 'UNION', 'LIFI', 'SYMBIOSIS'). This inconsistency could cause confusion for API consumers and violates the established naming convention for this enum. Consider renaming it to 'TEKS_STAGING' or 'TEKS-STAGING' to match the existing pattern.

Suggested change

export const SOURCES = ['FLYOVER', 'SWAP', 'POWPEG', 'UNION', 'LIFI', 'SYMBIOSIS', 'teks-staging'] as const;

export const SOURCES = ['FLYOVER', 'SWAP', 'POWPEG', 'UNION', 'LIFI', 'SYMBIOSIS', 'TEKS-STAGING'] as const;

Copilot · 2026-02-13T19:56:25Z

src/controllers/tx-history.controller.ts

+
+

There are two blank lines (96-97) in the middle of the method implementation. This appears to be unintentional and reduces code readability. Remove the extra blank line to maintain consistent code formatting.

Suggested change

Copilot · 2026-02-13T19:56:25Z

src/controllers/tx-history.controller.ts

+      }
+
+
+


The method verifyTransactionExistsOnBlockchain returns true for networks other than 'bitcoin' and 'rootstock', but the NETWORKS constant now includes 'Ethereum', 'BNB Smart Chain', and 'Lightning Network'. This means transactions from these networks will be accepted without any blockchain verification. This creates a security risk where invalid transaction hashes could be stored for these networks. Either implement verification for the newly-added networks or explicitly return false for unsupported networks to prevent storing unverified transactions.

Suggested change

}

} else {

this.logger.warn(

`[verifyTransactionExists] Unsupported network for verification: ${txHistory.fromNetworkName}`,

);

return false;

}

Copilot · 2026-02-13T19:56:25Z

src/controllers/tx-history.controller.ts

+        required: true,
+        schema: {
+          type: 'string',
+          pattern: '^[a-fA-F0-9]{64}$|^0x[a-fA-F0-9]{64}$',


The pattern ^[a-fA-F0-9]{64}$|^0x[a-fA-F0-9]{64}$ will always match with the first alternative ^[a-fA-F0-9]{64}$ for strings without the '0x' prefix because the regex alternation is evaluated left-to-right. However, due to the anchor ^ at the start of the second alternative, a string like '0x1234...' will fail the first alternative and then be tested by the second. The pattern works correctly but could be more efficiently written as ^(0x)?[a-fA-F0-9]{64}$. Consider refactoring for clarity and consistency with the pattern used in tx-history.model.ts line 29: ^(0x[a-fA-F0-9]{64}|[a-fA-F0-9]{64})$

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 10 comments.

Copilot · 2026-02-18T12:20:20Z

src/controllers/tx-history.controller.ts

+        txHistory.fromAmount = rskTransaction.value?.toString() ?? '';
+        txHistory.userAddress = rskTransaction.from?.toString() ?? '';
+      } else {
+        return null as unknown as TxHistory;


Using null as unknown as TxHistory to return a falsy value is an anti-pattern. The method should have a clearer return type. Consider changing the method signature to return Promise<boolean> and return true when the transaction exists and false when it doesn't, instead of using null casts. This would make the code more readable and type-safe.

Copilot · 2026-02-18T12:20:20Z

src/__tests__/acceptance/tx-history.acceptance.ts

+    it('should store a valid transaction and return 200', async () => {
+      const requestBody = getValidRequestBody();
+
+      storeTransactionStub = sinon.stub(TxHistoryService.prototype, 'storeTransaction').resolves(true);
+
+      const res = await client
+        .post('/tx-history')
+        .send(requestBody)
+        .expect(200);
+
+      expect(res.body).to.be.empty();
+      sinon.assert.calledOnce(storeTransactionStub);
+    });


The acceptance test stubs TxHistoryService.storeTransaction but does not stub BitcoinService.getTx or RskNodeService.getTransaction. Since the storeTransaction endpoint now calls verifyTransactionExistsOnBlockchain (which calls these services) before storing, the acceptance tests will likely fail or make real network calls. You should stub these services in the acceptance tests to prevent network calls and ensure tests are isolated.

Copilot · 2026-02-18T12:20:20Z

src/controllers/tx-history.controller.ts

+    return !!transaction;
+  }
+
+  private async verifyTransactionExistsOnBlockchain(txHistory: TxHistory): Promise<TxHistory> {


The method verifyTransactionExistsOnBlockchain returns TxHistory (or null cast as TxHistory), but it's being used as a boolean in line 57 with if (!transactionExists). When the method returns null (cast as TxHistory), this will be falsy and work as expected. However, when it returns a TxHistory object, the truthiness check will pass. The return type should be changed to Promise<boolean> or Promise<TxHistory | null>, and the usage should be updated accordingly. Additionally, the method mutates the input txHistory parameter (lines 93-94, 97-98) and returns it, which is confusing since the caller doesn't use the returned value except for the truthiness check.

Copilot · 2026-02-18T12:20:20Z

src/controllers/tx-history.controller.ts

+      this.logger.warn(
+        `[verifyTransactionExists] Transaction not found for ${txHistory.fromNetworkName}: ${txHistory.txHash} ${errorMessage}`,
+      );
+      return null as unknown as TxHistory;


Using null as unknown as TxHistory to return a falsy value is an anti-pattern. The method should have a clearer return type. Consider changing the method signature to return Promise<boolean> and return true when the transaction exists and false when it doesn't, instead of using null casts. This would make the code more readable and type-safe.

Copilot · 2026-02-18T12:20:21Z

src/controllers/tx-history.controller.ts

+    } catch (error: unknown) {
+      const errorMessage = error instanceof Error ? error.message : String(error);
+      this.logger.warn(
+        `[verifyTransactionExists] Transaction not found for ${txHistory.fromNetworkName}: ${txHistory.txHash} ${errorMessage}`,


The log message references '[verifyTransactionExists]' but the method name is 'verifyTransactionExistsOnBlockchain'. Update the log message to use the correct method name for consistency and easier debugging.

Suggested change

`[verifyTransactionExists] Transaction not found for ${txHistory.fromNetworkName}: ${txHistory.txHash} ${errorMessage}`,

`[verifyTransactionExistsOnBlockchain] Transaction not found for ${txHistory.fromNetworkName}: ${txHistory.txHash} ${errorMessage}`,

Copilot · 2026-02-18T12:20:21Z

src/models/tx-history.model.ts

 import {SearchableModel} from './rsk/searchable-model';

-export const SOURCES = ['FLYOVER', 'SWAP', 'POWPEG', 'UNION', 'LIFI', 'SYMBIOSIS'] as const;
+export const SOURCES = ['FLYOVER', 'SWAP', 'POWPEG', 'UNION', 'LIFI', 'SYMBIOSIS', 'teks-staging'] as const;


The value 'teks-staging' in SOURCES uses kebab-case while all other values use SCREAMING_SNAKE_CASE. This is inconsistent with the established naming pattern. Consider renaming to 'TEKS_STAGING' to maintain consistency.

Suggested change

export const SOURCES = ['FLYOVER', 'SWAP', 'POWPEG', 'UNION', 'LIFI', 'SYMBIOSIS', 'teks-staging'] as const;

export const SOURCES = ['FLYOVER', 'SWAP', 'POWPEG', 'UNION', 'LIFI', 'SYMBIOSIS', 'TEKS_STAGING'] as const;

Copilot · 2026-02-18T12:20:21Z

src/controllers/tx-history.controller.ts

+        txHistory.fromAmount = btcTransaction.amount.toString();
+        txHistory.userAddress = btcTransaction.address;


The code attempts to access btcTransaction.amount and btcTransaction.address properties (lines 93-94), but the BitcoinTx model does not define these properties. The BitcoinTx model only has properties like txid, vin, vout, valueOut, valueIn, etc. This will result in undefined values being assigned to txHistory.fromAmount and txHistory.userAddress, causing the verification to fail silently. You need to extract the amount and address from the available properties (e.g., from vout for amount and vin for address) or update the BitcoinService.getTx to return the required properties.

Suggested change

txHistory.fromAmount = btcTransaction.amount.toString();

txHistory.userAddress = btcTransaction.address;

const amount =

(btcTransaction as any).valueOut ??

((btcTransaction as any).vout

? (btcTransaction as any).vout.reduce(

(sum: number, output: any) => sum + (output?.value ?? 0),

0,

)

: undefined);

txHistory.fromAmount = amount != null ? amount.toString() : '';

const inputAddress =

(btcTransaction as any).vin?.[0]?.addr ??

(btcTransaction as any).vin?.[0]?.addresses?.[0] ??

(btcTransaction as any).vout?.[0]?.scriptPubKey?.addresses?.[0] ??

'';

txHistory.userAddress = inputAddress;

Copilot · 2026-02-18T12:20:21Z

src/controllers/tx-history.controller.ts

@@ -1,3 +1,4 @@
+/* eslint-disable no-param-reassign */


The eslint-disable directive for no-param-reassign is applied to the entire file, but parameter reassignment only occurs in the verifyTransactionExistsOnBlockchain method (lines 93-94, 97-98). Consider moving this directive to only disable the rule for the specific method or lines where it's needed, rather than the entire file.

Copilot · 2026-02-18T12:20:22Z

src/controllers/tx-history.controller.ts

@@ -89,11 +162,25 @@ export class TxHistoryController {
          },
        },
      },
+      '400': {
+        description: 'Invalid address or page parameter',
+      },
    },
  })
  async getTransactionHistory(
-    @param.query.string('address', {required: true}) address: string,
-    @param.query.number('page') page: number = 1,
+    @param.query.string('address', {
+      required: true,
+      schema: {
+        type: 'string',
+        pattern: '^(0x[a-fA-F0-9]{40}|[13mn][a-km-zA-HJ-NP-Z1-9]{25,34}|2[a-km-zA-HJ-NP-Z1-9]{25,34}|(bc1q|tb1q)[0-9a-z]{38,59}|(bc1p|tb1p)[0-9a-z]{39,59})$',
+      },
+    }) address: string,
+    @param.query.number('page', {
+      schema: {
+        type: 'number',
+        minimum: 1,
+      },
+    }) page: number = 1,
  ): Promise<Response> {
    try {
      if (!address) {
@@ -118,6 +205,18 @@ export class TxHistoryController {
  }

  @get('/tx-history/{txHash}', {
+    parameters: [
+      {
+        name: 'txHash',
+        in: 'path',
+        required: true,
+        schema: {
+          type: 'string',
+          pattern: '^[a-fA-F0-9]{64}$|^0x[a-fA-F0-9]{64}$',
+        },
+        description: 'Must be a valid transaction hash',
+      },
+    ],
    responses: {
      '200': {
        description: 'Transaction details',
@@ -127,13 +226,21 @@ export class TxHistoryController {
          },
        },
      },
+      '400': {
+        description: 'Invalid transaction hash format',
+      },
      '404': {
        description: 'Transaction not found',
      },
    },
  })
  async getTransactionByHash(
-    @param.path.string('txHash') txHash: string,
+    @param.path.string('txHash', {
+      schema: {
+        type: 'string',
+        pattern: '^[a-fA-F0-9]{64}$|^0x[a-fA-F0-9]{64}$',
+      },


The address validation pattern is duplicated in three places: in the parameters array (lines 113-123), in the @param decorator (lines 171-177), and in the GET endpoint parameters. The txHash pattern is also duplicated (lines 208-218 and 238-242). Consider extracting these patterns to constants to improve maintainability and reduce duplication.

Copilot · 2026-02-18T12:20:22Z

package.json

    "only-coverage": "lb-nyc report --reporter=lcov",
    "pretest": "npm run rebuild",
-    "test": "npm run unit-test",
+    "test": "npm run pretest && lb-nyc mocha --recursive 'dist/__tests__/**/*.unit.js' 'dist/__tests__/**/*.acceptance.js' --timeout 30000 --reporter spec",


The test script has been changed to run both unit and acceptance tests together instead of just unit tests. This changes the behavior of npm test to run a more comprehensive test suite. Ensure this change is intentional and that CI/CD pipelines are updated accordingly, as the test execution time will likely increase significantly.

Suggested change

"test": "npm run pretest && lb-nyc mocha --recursive 'dist/__tests__/**/*.unit.js' 'dist/__tests__/**/*.acceptance.js' --timeout 30000 --reporter spec",

"test": "npm run unit-test",

src/__tests__/acceptance/tx-history.acceptance.ts

+      const requestBody = getValidRequestBody();
+
+      storeTransactionStub = sinon.stub(TxHistoryService.prototype, 'storeTransaction').resolves(true);
+      const getTransactionByHashStub = sinon.stub(TxHistoryService.prototype, 'getTransactionByHash').resolves(null);


In general, to fix an unused variable where the expression’s side effect is required, you should keep the expression but remove the variable binding. Here, we still want to stub TxHistoryService.prototype.getTransactionByHash so that the real implementation is not called, but we do not need to store the stub instance in getTransactionByHashStub.

Concretely, in src/__tests__/acceptance/tx-history.acceptance.ts, within the "should store a valid transaction and return 200" test, replace the line that declares and assigns getTransactionByHashStub with a plain sinon.stub(...) call that is not assigned to any variable. No imports or other lines need to change, and we do not need to touch storeTransactionStub or any other code.

sonarqubecloud · 2026-02-18T14:28:45Z

Quality Gate passed

Issues
31 New issues
0 Accepted issues

Measures
0 Security Hotspots
90.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

test(tx-history): acceptance test for model

b1b1158

Adding model validation and tests for tx-history

ronaldsg20 force-pushed the sec/3.3.1 branch from 734e7fb to b1b1158 Compare January 30, 2026 20:10

test(tx-history): get methods schema

4993e2b

alexjavabraz requested a review from Copilot February 10, 2026 20:10

Copilot started reviewing on behalf of alexjavabraz February 10, 2026 20:10 View session

Copilot AI reviewed Feb 10, 2026

View reviewed changes

alexjavabraz added 2 commits February 13, 2026 14:26

validate transaction hash on respective blockchain networks

78d2c44

adding tx hash validation

93a0c50

Copilot AI review requested due to automatic review settings February 13, 2026 17:46

Copilot started reviewing on behalf of alexjavabraz February 13, 2026 17:47 View session

adjusting regex

31efe4a

Copilot AI reviewed Feb 13, 2026

View reviewed changes

alexjavabraz added 2 commits February 13, 2026 15:49

verify tx already exists

1b556af

adding one more provider

7e8f6a3

Copilot AI review requested due to automatic review settings February 13, 2026 19:52

Copilot started reviewing on behalf of alexjavabraz February 13, 2026 19:53 View session

Copilot AI reviewed Feb 13, 2026

View reviewed changes

alexjavabraz added 2 commits February 17, 2026 21:50

adding else clause

9f04517

persisting values from blockchain

1caf53d

Copilot AI review requested due to automatic review settings February 18, 2026 12:15

Copilot started reviewing on behalf of alexjavabraz February 18, 2026 12:15 View session

Copilot AI reviewed Feb 18, 2026

View reviewed changes

test(acceptance): TxHistoryController fix tests

bc54f68

Copilot AI review requested due to automatic review settings February 18, 2026 14:26

ronaldsg20 force-pushed the sec/3.3.1 branch from e87b06f to bc54f68 Compare February 18, 2026 14:26

github-advanced-security bot found potential problems Feb 18, 2026

View reviewed changes

Copilot started reviewing on behalf of ronaldsg20 February 18, 2026 14:42 View session

Copilot AI reviewed Feb 18, 2026

View reviewed changes

Copilot AI mentioned this pull request Feb 19, 2026

Fix tx-history tests: align with expanded enums and new blockchain verification logic #457

Draft

	export const SOURCES = ['FLYOVER', 'SWAP', 'POWPEG', 'UNION', 'LIFI', 'SYMBIOSIS', 'teks-staging'] as const;
	export const SOURCES = ['FLYOVER', 'SWAP', 'POWPEG', 'UNION', 'LIFI', 'SYMBIOSIS', 'TEKS-STAGING'] as const;

	`[verifyTransactionExists] Transaction not found for ${txHistory.fromNetworkName}: ${txHistory.txHash} ${errorMessage}`,
	`[verifyTransactionExistsOnBlockchain] Transaction not found for ${txHistory.fromNetworkName}: ${txHistory.txHash} ${errorMessage}`,

		txHistory.fromAmount = btcTransaction.amount.toString();
		txHistory.userAddress = btcTransaction.address;

-        txHistory.fromAmount = btcTransaction.amount.toString();
-        txHistory.userAddress = btcTransaction.address;
+        const amount =
+          (btcTransaction as any).valueOut ??
+          ((btcTransaction as any).vout
+            ? (btcTransaction as any).vout.reduce(
+                (sum: number, output: any) => sum + (output?.value ?? 0),
+,
+              )
+            : undefined);
+        txHistory.fromAmount = amount != null ? amount.toString() : '';
+        const inputAddress =
+          (btcTransaction as any).vin?.[0]?.addr ??
+          (btcTransaction as any).vin?.[0]?.addresses?.[0] ??
+          (btcTransaction as any).vout?.[0]?.scriptPubKey?.addresses?.[0] ??
+          '';
+        txHistory.userAddress = inputAddress;

	"test": "npm run pretest && lb-nyc mocha --recursive 'dist/__tests__/*/.unit.js' 'dist/__tests__/*/.acceptance.js' --timeout 30000 --reporter spec",
	"test": "npm run unit-test",

Conversation

ronaldsg20 commented Jan 30, 2026

Uh oh!

github-actions bot commented Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependency Review

OpenSSF Scorecard

Scanned Files

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 30, 2026 •

edited

Loading