Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ongrid, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a new mechanism to efficiently look up Request for Quotes (RFQs) by their topic hashes. This is achieved by adding a helper function to convert RFQ IDs (UUIDs) into keccak256 topic hashes and implementing a new thread-safe, time-expiring cache for RFQ messages, allowing for quick retrieval using these topic hashes.
Highlights
- New Helper Function: I've added a uuid_to_topic function in app/evm/helpers.py that converts a UUID into a keccak256 hash, suitable for use as an event topic in EVM contexts.
- RFQ Caching Mechanism: I've introduced a new RFQCache class in app/quoter/cache.py. This class provides a thread-safe, time-expiring cache (TTLCache) to store RFQMessage objects, indexed by their topic hashes.
- Efficient RFQ Lookup: The RFQCache includes add_rfq and get_rfq_by_topic methods, enabling the system to quickly store and retrieve RFQ messages using their computed topic hashes.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
ongrid
force-pushed
the
034_ttl_cache_for_rfqs_by_keccak_topic
branch
from
52694f8 to
e5e8946
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism for reverse-looking up RFQs by their topic hash. This is achieved by adding a uuid_to_topic helper function to generate a keccak256 hash from an RFQ's UUID, and a new RFQCache to store RFQs, keyed by this topic hash. The changes are well-tested and the new helper function and its test are implemented correctly. My main feedback is on the implementation of RFQCache, which is described as a singleton but not implemented as one, potentially leading to bugs. I've suggested a change to align the implementation with the singleton pattern.
| class RFQCache: | ||
| """Thread-safe expireable caching singleton for rfq data data.""" | ||
|
|
||
| _cache: TTLCache[HexBytes, RFQMessage] | ||
| _lock: asyncio.Lock | ||
|
|
||
| def __init__(self): | ||
| self._cache: TTLCache[HexBytes, RFQMessage] = TTLCache( | ||
| maxsize=CACHE_MAXSIZE, ttl=CACHE_TTL | ||
| ) | ||
| self._lock = asyncio.Lock() | ||
|
|
||
| async def add_rfq(self, rfq: RFQMessage): | ||
| """Add RFQ to the cache.""" | ||
| async with self._lock: | ||
| self._cache[uuid_to_topic(rfq.rfqId)] = rfq | ||
|
|
||
| async def get_rfq_by_topic(self, topic: HexBytes) -> Optional[RFQMessage]: | ||
| """Get RFQ by topic from the cache.""" | ||
| async with self._lock: | ||
| return self._cache.get(topic) |
There was a problem hiding this comment.
The docstring on line 15 states this class is a "singleton", but the implementation does not enforce this. If different parts of the application instantiate RFQCache, they will get separate cache instances, which is likely not the intended behavior and could lead to bugs.
To align with the singleton design, I suggest creating a single, shared instance of the cache at the module level. Consumers of this module can then import and use this shared instance. I've also taken the liberty to clean up the docstring for clarity and fix a minor typo ("data data").
class RFQCache:
"""Thread-safe, expiring cache for RFQ data."""
_cache: TTLCache[HexBytes, RFQMessage]
_lock: asyncio.Lock
def __init__(self):
self._cache: TTLCache[HexBytes, RFQMessage] = TTLCache(
maxsize=CACHE_MAXSIZE, ttl=CACHE_TTL
)
self._lock = asyncio.Lock()
async def add_rfq(self, rfq: RFQMessage):
"""Add RFQ to the cache."""
async with self._lock:
self._cache[uuid_to_topic(rfq.rfqId)] = rfq
async def get_rfq_by_topic(self, topic: HexBytes) -> Optional[RFQMessage]:
"""Get RFQ by topic from the cache."""
async with self._lock:
return self._cache.get(topic)
# A single, shared instance of the cache for the application to use.
rfq_cache = RFQCache()
ongrid
force-pushed
the
034_ttl_cache_for_rfqs_by_keccak_topic
branch
from
e5e8946 to
f1f6e1c
Compare
ongrid
force-pushed
the
034_ttl_cache_for_rfqs_by_keccak_topic
branch
from
f1f6e1c to
ba5d29d
Compare
|
/gemini review please |
There was a problem hiding this comment.
Pull Request Overview
This PR implements RFQ (Request for Quote) reverse lookup functionality by topic, adding a caching mechanism to efficiently store and retrieve RFQ messages. The implementation uses TTLCache for temporary storage with 5-minute expiration.
- Adds a TTL-based cache system for RFQ messages with configurable size and expiration
- Implements thread-safe async methods for storing and retrieving RFQ data by topic
- Provides comprehensive test coverage for cache lifecycle including TTL expiration behavior
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pyproject.toml | Adds cachetools dependency and type stubs for cache implementation |
| app/quoter/cache.py | Implements RFQCache class with TTL-based caching and async thread-safe operations |
| app/quoter/tests/test_quoter_rfq_cache.py | Comprehensive test suite covering cache lifecycle, TTL behavior, and expiration scenarios |
Comments suppressed due to low confidence (2)
pyproject.toml:10
- The cachetools version 6.1.0 does not exist. The latest stable version of cachetools is 5.x.x. Consider using a valid version range like "cachetools (>=5.3.0,<6.0.0)".
"cachetools (>=6.1.0,<7.0.0)",
pyproject.toml:15
- The types-cachetools version 6.1.0.20250717 does not exist. The types-cachetools package follows the pattern of the base package version. Consider using a valid version range like "types-cachetools (>=5.3.0,<6.0.0)".
"types-cachetools (>=6.1.0.20250717,<7.0.0.0)",
| @@ -0,0 +1,117 @@ | |||
| """Tests for health checker.""" | |||
There was a problem hiding this comment.
The docstring incorrectly states "Tests for health checker" but this file contains tests for RFQ cache functionality. It should be "Tests for RFQ cache."
| """Tests for health checker.""" | |
| """Tests for RFQ cache.""" |
|
|
||
|
|
||
| class RFQCache: | ||
| """Thread-safe expireable caching singleton for rfq data data.""" |
There was a problem hiding this comment.
The word "expireable" should be "expirable", and there's a duplicate word "data data" that should be "data".
| """Thread-safe expireable caching singleton for rfq data data.""" | |
| """Thread-safe expirable caching singleton for rfq data.""" |
There was a problem hiding this comment.
Code Review
This pull request introduces a time-to-live (TTL) cache for RFQs, allowing them to be looked up by topic. The implementation uses cachetools and asyncio.Lock for coroutine-safe caching. The changes are accompanied by a comprehensive test suite that validates the cache's lifecycle, including item expiration. My review focuses on improving code clarity and maintainability in both the cache implementation and its tests. I've suggested updates to docstrings to make them more accurate and recommended refactoring the tests to reduce code duplication.
|
|
||
|
|
||
| class RFQCache: | ||
| """Thread-safe expireable caching singleton for rfq data data.""" |
There was a problem hiding this comment.
The docstring contains a typo ("data data") and inaccurately describes the class as a singleton. While it's intended to be used as a single instance, the class itself doesn't enforce the singleton pattern. It would be clearer to describe it as a coroutine-safe cache to avoid confusion.
| """Thread-safe expireable caching singleton for rfq data data.""" | |
| """Coroutine-safe, time-expiring cache for RFQ data.""" |
| @@ -0,0 +1,117 @@ | |||
| """Tests for health checker.""" | |||
There was a problem hiding this comment.
The docstring for this test module appears to be a copy-paste from another file. It should describe the purpose of these tests, which is to test the RFQ cache.
| """Tests for health checker.""" | |
| """Tests for the RFQ cache.""" |
|
|
||
| rfq_cache = RFQCache() | ||
| yield mock_controller, rfq_cache | ||
|
|
There was a problem hiding this comment.
To avoid duplicating the start_time value, this fixture can be updated to yield the start_time along with the mock controller and cache instance. This will allow the test to get this value from the fixture directly. See the related comment in test_cache_lifecycle.
| yield mock_controller, rfq_cache, start_time |
| start_time = 1754000000.0 | ||
| mock_time.current_time = start_time | ||
| # Check initial state |
There was a problem hiding this comment.
The start_time is hardcoded here, duplicating the value from the rfq_cache_with_time_mock fixture. With the suggested change to the fixture, you can unpack the start_time here and remove the hardcoded value and redundant time setting.
| start_time = 1754000000.0 | |
| mock_time.current_time = start_time | |
| # Check initial state | |
| mock_time, rfq_cache, start_time = rfq_cache_with_time_mock |
/gemini review
closes #34