chore(currency): search by ID from Map #1465
Conversation
WalkthroughThe changes introduce a new private property, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CurrencyManager
participant Cache
Client->>CurrencyManager: fromId("USDC-multichain-moonbeam")
CurrencyManager->>Cache: Check knownCurrenciesById
alt Currency found in cache
Cache-->>CurrencyManager: Return CurrencyDefinition
CurrencyManager-->>Client: Return CurrencyDefinition
else Currency not found
CurrencyManager->>CurrencyManager: Search knownCurrencies
CurrencyManager->>Cache: Cache CurrencyDefinition
CurrencyManager-->>Client: Return CurrencyDefinition
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
requested review from
alexandre-abrioux,
leoslr,
MantisClone and
sstefdev
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- packages/currency/src/currencyManager.ts (2 hunks)
- packages/currency/test/currencyManager.test.ts (1 hunks)
🔇 Additional comments (1)
packages/currency/src/currencyManager.ts (1)
23-25: Implementation of caching mechanism looks goodThe addition of
knownCurrenciesByIdMap initializes correctly.
kevindavee
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- packages/currency/src/currencyManager.ts (3 hunks)
- packages/currency/test/currencyManager.test.ts (1 hunks)
🔇 Additional comments (5)
packages/currency/src/currencyManager.ts (4)
23-24: LGTM: New property for caching currency definitionsThe addition of the
knownCurrenciesByIdproperty as aMapis a good choice for caching currency definitions by their IDs. This will allow for O(1) lookup time, addressing the performance issue mentioned in the PR objectives.
47-49: LGTM: Efficient initialization ofknownCurrenciesByIdThe initialization of
knownCurrenciesByIdin the constructor is implemented efficiently, using a single pass overknownCurrencies. This approach ensures that the cache is fully populated at construction time, providing immediate O(1) lookups for all known currencies. This change effectively addresses the suggestion from the past review comment.
93-93: LGTM: OptimizedfromIdmethod for O(1) lookupThe
fromIdmethod has been simplified to directly use theknownCurrenciesByIdMap, resulting in O(1) lookup time. This change significantly improves the method's performance, especially when handling multiple currency lookups, as mentioned in the PR objectives. The elimination of the fallback linear search is appropriate since the cache is now pre-populated in the constructor.
23-24: LGTM: Effective implementation of caching for improved performanceThe changes introduced in this PR successfully implement a caching mechanism for currency definitions using a Map. The cache is efficiently populated at construction time and utilized in the
fromIdmethod, resulting in O(1) lookup time for currency definitions. These modifications effectively address the performance issue described in the PR objectives, particularly for scenarios involving multiple currency lookups.The implementation is well-structured, maintains the integrity of the
CurrencyManagerclass, and addresses the suggestions from the past review comment. Overall, these changes should significantly improve the efficiency of currency-related operations in the system.Also applies to: 47-49, 93-93
packages/currency/test/currencyManager.test.ts (1)
156-161: Good addition to the test suite.This new test case enhances the coverage of the
fromIdmethod and aligns well with the PR objectives of implementing caching for the.fromIdresults. It integrates seamlessly into the existing test structure without disrupting other tests.As the caching functionality evolves, consider adding more comprehensive tests for various caching scenarios, such as cache expiration (if implemented) or cache invalidation.
|
Reviewing... 🚧 |
Description of the changes
Search currency with
.fromIdresults from aMap.Since Node.js is a single thread, we couldn't loop through an array in parallel.
In the above example, even though we use
Promise.all, the whole operation will take (200* 20ms) + 10ms = 4010ms. The async task could be executed concurrently, but the array iteration runs serially. If we cache the result in aMap, we could get the data withO(1)time complexity.Summary by CodeRabbit