Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds memoization decorators to several service methods, changes getTransactionsByAddress to positional params and updates its repository SQL to use an INNER JOIN with addressReference, denormalizes/backfills a height column on addressReference with a composite index, updates indexer code and tests to populate and account for height. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant AddressSvc as AddressService
participant TxSvc as TransactionService
participant Repo as TransactionRepository
participant DB as Database
Note over AddressSvc: getAddressDetails is memoized (ttl=averageBlockTime)
Client->>AddressSvc: GET /address/:addr/details
AddressSvc->>AddressSvc: check cache
alt cache hit
AddressSvc-->>Client: return cached response
else cache miss
AddressSvc->>TxSvc: getTransactionsByAddress(address, 0, 5)
TxSvc->>TxSvc: check cache
alt tx cache hit
TxSvc-->>AddressSvc: cached transactions
else tx cache miss
TxSvc->>Repo: getTransactionsByAddress(address, skip, limit)
Repo->>DB: SELECT ... INNER JOIN addressReference ON ... WHERE address = $1 ORDER BY af.height DESC, t.index DESC
DB-->>Repo: rows
Repo-->>TxSvc: mapped transactions
TxSvc-->>TxSvc: store in cache
TxSvc-->>AddressSvc: transactions
end
AddressSvc->>AddressSvc: compose response (include txs)
AddressSvc-->>Client: response
AddressSvc-->>AddressSvc: store in cache
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/api/src/transaction/services/transaction/transaction.service.ts`:
- Around line 22-25: The Memoize decorator is generating identical keys because
getTransactionsByAddress receives a single object argument which is filtered
out; fix by changing the method signature to accept primitive args (e.g., async
getTransactionsByAddress(address: string, skip?: number, limit?: number) and
update callers to pass address, skip, limit) so Memoize will include them in the
cache key, or alternatively update/replace the Memoize usage on
getTransactionsByAddress to supply a custom cache-key generator that serializes
the object (address, skip, limit) into a stable string before calling
this.transactionRepository.getTransactionsByAddress; locate the method
getTransactionsByAddress in transaction.service and the Memoize decorator usage
to implement one of these two fixes.
🧹 Nitpick comments (1)
apps/api/src/validator/services/validator/validator.service.ts (1)
46-46: Avoid usinganytype.The
error: anytype annotation violates the coding guidelines. Consider usingunknownand performing proper type narrowing, or use a more specific error type.Suggested fix
- } catch (error: any) { - if (error.response?.status === 404) { + } catch (error: unknown) { + if (error instanceof Error && "response" in error && (error as { response?: { status?: number } }).response?.status === 404) {Alternatively, if using axios errors:
+import { AxiosError } from "axios"; ... - } catch (error: any) { - if (error.response?.status === 404) { + } catch (error) { + if (error instanceof AxiosError && error.response?.status === 404) {As per coding guidelines, never use type
any.
apps/api/src/transaction/services/transaction/transaction.service.ts
Outdated
Show resolved
Hide resolved
… in caching decorators
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (79.31%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2558 +/- ##
==========================================
- Coverage 50.73% 50.44% -0.29%
==========================================
Files 1063 1054 -9
Lines 29513 29185 -328
Branches 6541 6507 -34
==========================================
- Hits 14973 14723 -250
+ Misses 14187 14109 -78
Partials 353 353
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/transaction/repositories/transaction/transaction.repository.ts (1)
102-123: Potential SQL error whenskiporlimitisundefined.The parameters
skipandlimitare optional (number | undefined). If either isundefined, the raw SQL query will receiveundefinedas a replacement value, which could cause unexpected behavior or errors (e.g.,OFFSET undefined LIMIT undefined).Consider providing default values:
🔧 Suggested fix
- async getTransactionsByAddress(address: string, skip?: number, limit?: number): Promise<GetAddressTransactionsResponse> { + async getTransactionsByAddress(address: string, skip: number = 0, limit: number = 20): Promise<GetAddressTransactionsResponse> {
🤖 Fix all issues with AI agents
In `@apps/api/src/transaction/services/transaction/transaction.service.ts`:
- Around line 23-26: The cache key collision is caused by optional skip/limit
being undefined and omitted from the Memoize key; change the signature of
getTransactionsByAddress to use numeric default sentinels (e.g., skip: number =
-1, limit: number = -1) so both parameters are always numbers and included in
the Memoize key, and when calling
this.transactionRepository.getTransactionsByAddress(map the sentinel -1 back to
undefined if the repository expects optional parameters) — update the function
signature and the forwarding call accordingly (referencing
getTransactionsByAddress and the Memoize decorator).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Performance Improvements
Database & Indexing
✏️ Tip: You can customize this high-level summary in your review settings.