feat(transactions): add multi-safe queued transactions endpoint#2985
feat(transactions): add multi-safe queued transactions endpoint#2985clovisdasilvaneto wants to merge 5 commits intomainfrom
Conversation
Implement endpoint to retrieve queued transactions across multiple Safes in a single request. Transactions are fetched in batches, merged, and sorted by timestamp. Returns top N transactions for each Safe address. - Add SafeQueuedTransaction entity wrapping transaction with safe address and chain ID - Add GET /multi-safe/transactions/queued endpoint accepting CAIP-10 formatted addresses - Implement getMultiSafeTransactionQueue service with configurable batch size - Add integration tests for multi-safe transaction queue endpoint - Support filtering by trust status and limiting results Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
0ef85a2 to
20a3951
Compare
- Remove queueService property that doesn't exist in actual configuration - Simplify failing multi-safe test to avoid complex mock setup
LucieFaire
left a comment
There was a problem hiding this comment.
its not the most performant method (and for a large set it can grow exponentialy) and it could be improved, but if its a temp solution it is a solid one. I left some comments.
It would also be great to have unit tests for service-level method as there is a lot of logic going on
| return new TXSCreationTransaction(tx); | ||
| } | ||
|
|
||
| private static readonly MULTI_SAFE_QUEUE_BATCH_SIZE = 3; |
There was a problem hiding this comment.
not: that would usually go to the top of the file, before the constructor
| readonly safeAddress: string; | ||
| @ApiProperty() | ||
| readonly chainId: string; | ||
| @ApiProperty() |
There was a problem hiding this comment.
@ApiProperty({ type: Transaction })
so that Swagger can infer the type fully
| export class SafeQueuedTransaction { | ||
| @ApiProperty() | ||
| readonly safeAddress: string; | ||
| @ApiProperty() | ||
| readonly chainId: string; | ||
| @ApiProperty() | ||
| readonly transaction: Transaction; | ||
|
|
||
| constructor(safeAddress: string, chainId: string, transaction: Transaction) { | ||
| this.safeAddress = safeAddress; | ||
| this.chainId = chainId; | ||
| this.transaction = transaction; | ||
| } |
There was a problem hiding this comment.
you can also write it in a short form:
export class SafeQueuedTransaction {
@ApiProperty()
readonly safeAddress!: string;
@ApiProperty()
readonly chainId!: string;
@ApiProperty({ type: Transaction })
readonly transaction!: Transaction;
}
! means the definite assignment assertion (!) which tells TypeScript the properties will be assigned in the constructor.
| safe, | ||
| dataDecoded, | ||
| ); | ||
| return new SafeQueuedTransaction(address, chainId, transaction); |
There was a problem hiding this comment.
if you update the DTO definition, you can just return plain object w/o contructor
return {
safeAddress: address,
chainId,
transaction,
};
```s
| const queue = await this.safeRepository.getTransactionQueue({ | ||
| chainId, | ||
| safe, | ||
| limit: args.limit, | ||
| offset: 0, | ||
| trusted: args.trusted, | ||
| }); |
There was a problem hiding this comment.
Each Safe fetches args.limit transactions (defaults to 10), but the final result is also capped to args.limit. With 15 Safes and limit=10, you could fetch up to 150 transactions, decode and map all of them, then throw away 140.
Since results are sorted by timestamp (oldest first), you could:
Fetch a smaller per-Safe limit (like that batch size)
or after each batch, track the current "worst" timestamp and skip further processing if a Safe's transactions can't make the cut
There was a problem hiding this comment.
@LucieFaire indeed, I can indeed fetch smaller number of pending tx per safe, but this could also lead to the following scenario:
Suppose we fetch 5 per safe, but the user asked for a limit of 15 txs and he just asked for one safe which has 20 pending tx... In this case e would only have 5 pending txs on the response, which would be wrong in this case.
There was a problem hiding this comment.
dont you have control over what is returned? Are you planning to give user control over how much data they want and for what safes?
There was a problem hiding this comment.
Yes I do, but I'm just talking in a general scenario, in case it is gonna be used elsewhere, but yeah, maybe it might not even make sense, since this is a temporary approach.
|
|
||
| private static readonly MULTI_SAFE_QUEUE_BATCH_SIZE = 3; | ||
|
|
||
| async getMultiSafeTransactionQueue(args: { |
There was a problem hiding this comment.
If multiple users in the same Space hit this endpoint simultaneously for the same set of Safes, each request does the full work independently. There is caching on separate methods used (like transactions ,and safe) but maybe worth adding caching for this set of params too
|
@claude review |
|
Claude finished @PooyaRaki's task —— View job Code Review: PR #2985
Issues to Address1. No upper bound on 2. Over-fetching per Safe ( 3. Sequential batch processing negates concurrency benefits ( 4. Missing 5. Static constant placement ( 6. Missing unit tests for service logic 7. No test for | |
| description: | ||
| 'Comma-separated list of Safe addresses in CAIP-10 format (chainId:address)', | ||
| example: | ||
| '1:0x1234567890123456789012345678901234567890,5:0xabcdefabcdefabcdefabcdefabcdefabcdefabcd', |
There was a problem hiding this comment.
QQ: Why don't we accept an object with chainId as the key which has safeAddresses as the values?
e.g.
{
"1" : [{{ADDRESS_1}}, {{ADDRESS_2}}],
"5": [{{ADDRESS_1}}, {{ADDRESS_2}}],
}There was a problem hiding this comment.
@PooyaRaki because I'm following the same standard we have in the safesOverview endpoint (the one we use to get the number of queued transactions and show in the sidebar)
| description: | ||
| 'Array of queued transactions enriched with Safe address and chain ID', | ||
| }) | ||
| @Get('multi-safe/transactions/queued') |
There was a problem hiding this comment.
What do you think removing multi-safe? then it would be
@Get('transactions/queued')
| const limitedSafes = args.addresses.slice(0, this.maxOverviews); | ||
| const transactions: Array<SafeQueuedTransaction> = []; | ||
|
|
||
| for ( |
There was a problem hiding this comment.
Instead of using a for loop and then .slice we could simplify it using lodash which is already a dependency, Something like the following:
for (const batch of chunk(limitedSafes, TransactionsService.MULTI_SAFE_QUEUE_BATCH_SIZE)) {
| networkService = moduleFixture.get(NetworkService); | ||
| loggingService = moduleFixture.get(LoggingService); | ||
|
|
||
| jest.spyOn(loggingService, 'error'); |
There was a problem hiding this comment.
Since the service is mocked we can safely remove the mocks.
| ...baseConfiguration.mappings, | ||
| safe: { | ||
| ...baseConfiguration.mappings.safe, | ||
| maxOverviews: 10, |
There was a problem hiding this comment.
Should use a random number generated by faker
| await app.close(); | ||
| }); | ||
|
|
||
| async function buildSafeWithTransaction(args: { |
There was a problem hiding this comment.
Nit: We could extract the builder to another file.
| submissionDate: new Date('2024-01-01T00:00:00Z'), | ||
| }); | ||
| const safe2Data = await buildSafeWithTransaction({ | ||
| nonce: 1, |
There was a problem hiding this comment.
Nit: We should use faker here.
| }); | ||
| const safe2Data = await buildSafeWithTransaction({ | ||
| nonce: 1, | ||
| submissionDate: new Date('2024-06-01T00:00:00Z'), |
There was a problem hiding this comment.
We can use faker to generate the dates throughout the test file.
| networkService.get.mockImplementation(({ url }) => { | ||
| for (const data of allMocks) { | ||
| const chainUrl = `${safeConfigUrl}/api/v1/chains/${data.chain.chainId}`; | ||
| const safeUrl = `${data.chain.transactionService}/api/v1/safes/${data.safeAddress}`; | ||
| const txUrl = `${data.chain.transactionService}/api/v2/safes/${data.safeAddress}/multisig-transactions/`; | ||
|
|
||
| if (url === chainUrl) { | ||
| return Promise.resolve({ | ||
| data: rawify(data.chain), | ||
| status: 200, | ||
| }); | ||
| } | ||
| if (url === safeUrl) { | ||
| return Promise.resolve({ | ||
| data: rawify(data.safe), | ||
| status: 200, | ||
| }); | ||
| } | ||
| if (url === txUrl) { | ||
| return Promise.resolve({ | ||
| data: rawify({ | ||
| count: 1, | ||
| next: null, | ||
| previous: null, | ||
| results: [ | ||
| multisigToJson(data.transaction) as MultisigTransaction, | ||
| ], | ||
| }), | ||
| status: 200, | ||
| }); | ||
| } | ||
| } | ||
| if (url.includes('/api/v1/tokens/')) { | ||
| return Promise.resolve({ data: rawify({}), status: 200 }); | ||
| } | ||
| if (url.includes('/api/v1/contracts/')) { | ||
| return Promise.resolve({ | ||
| data: rawify( | ||
| pageBuilder().with('results', [contractBuilder().build()]).build(), | ||
| ), | ||
| status: 200, | ||
| }); | ||
| } | ||
| if (url.includes('/api/v1/safe-apps/')) { | ||
| return Promise.resolve({ data: rawify([]), status: 200 }); | ||
| } | ||
| return Promise.reject(new Error(`Could not match ${url}`)); | ||
| }); |
There was a problem hiding this comment.
We should extract all the mock within the tests to another method to keep the test itself short and easy to follow.
|
|
||
| const safesParam = `${chain.chainId}:${safeAddress}`; | ||
|
|
||
| await request(app.getHttpServer()) |
There was a problem hiding this comment.
We should explicitly check trusted and limit in this test as the title suggests.
- Remove 'multi-safe' prefix from queued transactions endpoint - Simplify batch processing using lodash chunk() utility - Extract buildSafeWithTransaction helper to separate builder file - Use faker consistently for test data generation - Update entity class to use property initialization - Add multiSafeQueue configuration for cache expiration
46287cf to
4f99075
Compare
Summary
Implement an endpoint to retrieve pending queued transactions across multiple Safe addresses in a single request. This enables efficient monitoring of transaction queues for multiple Safes without making separate requests. We're gonna use this endpoint in the spaces dashboard while queue-service doesn't support mainnet.
Changes
safes(required): Comma-separated CAIP-10 addresses (e.g.,1:0x1234...,5:0xabcd...)trusted(optional): Filter by trust status (default: true)limit(optional): Max transactions per result (default: 10)🤖 Generated with Claude Code