Skip to content

Conversation

PooyaRaki
Copy link
Contributor

Summary

This refactor updates the API version used by the multisig transactions and all-transactions endpoints to ensure compatibility with the latest backend changes.

Changes

  • Bumped API version for multisig transactions endpoint
  • Bumped API version for all-transactions endpoint
  • Updated related request/response type definitions (where necessary)
  • Adjusted tests to align with the new API version

@PooyaRaki PooyaRaki requested a review from a team as a code owner September 3, 2025 11:06
@PooyaRaki PooyaRaki self-assigned this Sep 3, 2025
@PooyaRaki
Copy link
Contributor Author

@claude review

Copy link

claude bot commented Sep 4, 2025

Claude finished @PooyaRaki's task —— View job


Code Review Complete

I've reviewed the API version refactor for multisig transactions and all-transactions endpoints. Here's my feedback:

✅ Positive Aspects

Consistent API Migration: The changes systematically update 7 transaction-related endpoints from v1 to v2, maintaining consistency across:

  • getMultisigTransactions() - src/datasources/transaction-api/transaction-api.service.ts:658
  • getMultisigTransactionsWithNoCache() - src/datasources/transaction-api/transaction-api.service.ts:720
  • getMultisigTransaction() - src/datasources/transaction-api/transaction-api.service.ts:751
  • getMultisigTransactionWithNoCache() - src/datasources/transaction-api/transaction-api.service.ts:767
  • deleteTransaction() - src/datasources/transaction-api/transaction-api.service.ts:782
  • getAllTransactions() - src/datasources/transaction-api/transaction-api.service.ts:851
  • postMultisigTransaction() - src/datasources/transaction-api/transaction-api.service.ts:1024

Comprehensive Test Coverage: All 15 test files properly updated to reflect the new API versions, ensuring no test failures.

⚠️ Areas for Improvement

Mixed API Versions: The codebase now has mixed v1/v2 usage which could be confusing:

  • Confirmations still use v1: api/v1/multisig-transactions/${args.safeTxHash}/confirmations/ (line 553)
  • Estimations remain v1: api/v1/safes/${args.address}/multisig-transactions/estimations/ (line 955)

Consider documenting why some endpoints remain on v1 or planning their migration.

Cache Key Consistency: Verify that cache keys still work correctly after endpoint changes, as cache invalidation hooks may reference specific API versions.

Missing Error Handling: No additional error handling for potential v1/v2 API differences in response formats or error codes.

📝 Minor Issues

  1. Documentation: Consider updating any API documentation or OpenAPI specs to reflect the version changes
  2. Monitoring: Ensure logging/metrics can distinguish between v1 and v2 endpoint usage for monitoring purposes

Summary

This is a well-executed refactor with good test coverage. The changes appear safe and maintain backward compatibility through proper test updates. The mixed v1/v2 state is the main concern but may be intentional based on backend migration timelines.


@PooyaRaki PooyaRaki requested a review from LucieFaire September 4, 2025 11:36
@PooyaRaki PooyaRaki merged commit 9eb36c4 into main Sep 9, 2025
27 of 28 checks passed
@PooyaRaki PooyaRaki deleted the feat/TransactionV2 branch September 9, 2025 10:23
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.

2 participants