Skip to content

Conversation

@pedroanastacio
Copy link
Member

@pedroanastacio pedroanastacio commented Jan 20, 2026

Description

Some API tests had errors:

  • GET /transaction/pending should get transactions pending
  • GET /user/transactions should accept both status and type query params

Summary

  • GET /user/transactions should accept both status and type query params: send query param status as an array
  • GET /transaction/pending should get transactions pending:
    • use an isolated vault to check pending transactions
    • creates logic for incremental transaction counting (previousCount + 1), since the count of pending transactions is per user

Checklist

  • I reviewed my PR code before submitting
  • I ensured that the implementation is working correctly and did not impact other parts of the app
  • I mentioned the PR link in the task

.set('Authorization', users[0].token)
.set('Signeraddress', users[0].payload.address)
.send(payload_transfer);
const vault = predicates[5];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 CRITICAL: Hardcoded array index without bounds validation

Problem:
Using predicates[5] assumes the array has at least 6 elements, but the test environment is initialized with only 6 predicates (index 0-5). This creates a tight coupling and potential runtime error if the initialization changes.

Suggestion:
Add bounds checking or use a more defensive approach:

const vault = predicates[predicates.length - 1]; // Use last predicate
// or
if (predicates.length < 6) throw new Error('Not enough predicates for test');
const vault = predicates[5];

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted on: a3497e0

assert.equal(res.status, 200);
assert.ok('ofUser' in res.body);
assert.ok('transactionsBlocked' in res.body);
assert.ok('pendingSignature' in res.body);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 IMPORTANT: Missing validation for incremental counting assumption

Problem:
The test assumes that saveMockTransaction will always create exactly one pending transaction, but there's no validation that the mock transaction was actually created with PENDING status.

Suggestion:
Add validation to ensure the mock transaction is in pending state:

// After saveMockTransaction
const createdTx = await saveMockTransaction({ vault, user }, app);
assert.equal(createdTx.status, TransactionStatus.PENDING_SENDER);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted on: 4658068

Copy link
Member

@guimroque guimroque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review - Summary

What was done

Fixed two failing API tests:

  1. GET /transaction/pending: Improved test isolation by using a dedicated vault (predicates[5]) and implementing incremental transaction counting logic
  2. GET /user/transactions: Fixed query parameter format to properly send status as an array using bracket notation

Positive Points

  • Good test isolation approach using a separate vault for pending transactions test
  • Smart incremental counting logic (previousCount + 1) makes the test more robust
  • Proper query parameter format for array values
  • Clean removal of unused import (ZeroBytes32)
  • Consistent code formatting and structure

Issues Found

  • One critical issue: hardcoded predicate index without bounds checking could cause runtime errors
  • One important issue: missing validation for the incremental counting assumption

Total comments: 2 (1 critical, 1 important)

Copy link
Member

@guimroque guimroque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! ✅

Previous issues have been fixed. Code approved.

@pedroanastacio pedroanastacio merged commit 4cbb45c into staging Jan 20, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants