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
@@ -122,6 +122,7 @@ export class ZerionPositionsApi implements IPositionsApi {
);
return this._mapPositions(chainName, zerionBalances.data);
} catch (error) {
console.log('error', error);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was added by mistake, thanks for flagging, removed ✅
e5eb4c6

@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
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