Conversation
📝 WalkthroughWalkthroughRemoves RPC- and NFT-related modules, code, and tests; adds a typed Blokli GraphQL provider and Blokli providers/models; converts many timing/config fields to duration types; updates config parsers, mixins, core components, packaging, Dockerfile, and tests to use Blokli and duration-based semantics. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant BlokliProv as BlokliProvider
participant HTTP as aiohttp.ClientSession
participant BlokliAPI as Blokli API
App->>BlokliProv: get(**kwargs)
activate BlokliProv
BlokliProv->>BlokliProv: _initialize_query / _load_query(query_file)
BlokliProv->>BlokliProv: _get(**kwargs)
BlokliProv->>HTTP: POST /graphql (url, headers, body)
activate HTTP
HTTP->>BlokliAPI: Execute GraphQL query
activate BlokliAPI
BlokliAPI-->>HTTP: JSON response + headers
deactivate BlokliAPI
HTTP-->>BlokliProv: response dict, headers
deactivate HTTP
BlokliProv->>BlokliProv: extract data and convert to model
BlokliProv-->>App: Typed JsonResponse (e.g., BlokliSafe)
deactivate BlokliProv
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…t/ct-research into jean/fix-api-usage-for-v4
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
ct-app/core/components/messages/message_format.py (1)
86-90:⚠️ Potential issue | 🟡 MinorKeep
timestampas a string to preserve format contract.Widening the type to
int | strencourages storing an int, which conflicts with the MessageFormat pattern contract where timestamp is expected as a string. Please keep it as a string (even when deriving “now” or parsing a numeric input). Based on learnings: “In the MessageFormat class (ct-app/core/components/messages.py), the timestamp should be kept as a string to match the pattern format. The timestamp is defined in the class pattern and is extracted during parsing, so it exists when the object is created.”🔧 Suggested fix
- def update_timestamp(self, timestamp: Optional[int | str] = None): - if timestamp is not None: - self.timestamp = int(float(timestamp)) - else: - self.timestamp = int(datetime.now().timestamp() * 1000) + def update_timestamp(self, timestamp: Optional[str] = None): + if timestamp is not None: + self.timestamp = str(int(float(timestamp))) + else: + self.timestamp = str(int(datetime.now().timestamp() * 1000))ct-app/test/pyproject.toml (1)
4-4:⚠️ Potential issue | 🟠 MajorPython 3.14 requirement is inconsistent with the root package and unreleased.
requires-python = ">=3.14"specifies an unreleased Python version (currently in alpha). This requirement is consistent across multiple submodules but conflicts with the rootct-app/pyproject.tomlwhich requires>=3.12. Clarify whether this higher version requirement for the test submodule is intentional or if it should align with the root package's>=3.12.ct-app/test/components/test_utils.py (1)
61-84:⚠️ Potential issue | 🟡 MinorAlign test input type with the updated mergeDataSources signature.
mergeDataSourcesnow expects aset[Peer]; passing a list here diverges from production usage and skips set semantics (dedupe/hash).💡 Suggested change
- peers_list = [ - Peer("address_1"), - Peer("address_2"), - Peer("address_3"), - ] + peers_list = { + Peer("address_1"), + Peer("address_2"), + Peer("address_3"), + }ct-app/core/components/peer.py (1)
101-121:⚠️ Potential issue | 🔴 Critical
is_eligible()can raiseValueErrorwhen balances aren't set, breaking theboolreturn contract.The
split_stakeproperty raisesValueErrorifsafe.balanceorchannel_balanceisNone(lines 76–81). The currentis_eligible()method only checksif not self.safebefore accessingsplit_stakeat line 120, leaving it vulnerable to exceptions. Add a guard to returnFalsewhen these balance fields are unset:Return False when required balances are missing
if not self.safe: return False + + if self.safe.balance is None or self.channel_balance is None: + return False
🤖 Fix all issues with AI agents
In `@ct-app/.configs/core_prod_config.yaml`:
- Around line 123-125: Add environment variable injection calls for blokli by
invoking set_attribute_from_env on the params.blokli object (matching how
subgraph.api_key is handled); specifically, call
params.blokli.set_attribute_from_env("url", "BLOKLI_URL") and
params.blokli.set_attribute_from_env("token", "BLOKLI_TOKEN") in the application
startup sequence (near where params and other external services are initialized,
e.g., alongside the existing subgraph API key setup in the __main__
initialization) so BLOKLI_URL and BLOKLI_TOKEN can populate blokli.url and
blokli.token from the environment.
In `@ct-app/core/api/hoprd_api.py`:
- Around line 84-93: The return type annotation of try_req currently omits
response types allowed by the overloads (bool and list); update the
implementation signature for try_req to match the overloads by widening the
return type to include bool and list (e.g., Optional[Union[Response, dict, list,
bool]] or Optional[Any]) so static type checkers accept all overload-returned
values, and ensure any necessary typing imports (list or Any) are present;
reference the async method name try_req to locate the change.
In `@ct-app/core/blokli/blokli_provider.py`:
- Around line 86-89: The constructor stores a token (self.token) but it isn't
sent with requests: update the request to include an Authorization: Bearer
<token> header (or remove the token parameter if you prefer). Concretely, when
calling aiohttp.ClientSession() / session.post(self.url, json=...), supply
headers that include "Authorization": f"Bearer {self.token}" only when
self.token is set (or build the session with those headers); ensure you
reference the stored field self.token and the existing session.post/self.url
call so authenticated Blokli deployments receive the header.
- Around line 81-84: The logger calls in blokli_provider.py pass a dict as a
positional arg (e.g., logger.debug("Executing blokli query", {"url": self.url,
"query": query, "variables": variable_values})) which triggers string-formatting
errors; update each such call (the debug/info/error calls around the occurrences
you noted) to pass the structured dict via the extra= keyword (e.g.,
extra={"url": self.url, ...}) instead of as a positional argument; search for
logger.* calls in the BlokliProvider methods (those using self.url, query,
variable_values, response, etc.) and replace the second positional dict argument
with extra= to prevent JSONFormatter formatting issues.
In `@ct-app/core/components/config_parser/blokli.py`:
- Around line 6-9: Override BlokliParams.__repr__ to avoid logging the raw
token: implement __repr__ in the BlokliParams class (which inherits
ExplicitParams) to build a copy of vars(self), replace the "token" value with a
masked string (e.g. "<redacted>" or "****"), and return a class-styled
representation (for example "BlokliParams({...})") using that masked dict;
reference the BlokliParams class and the __repr__ method and ensure the
attribute name "token" is the one being masked so Parameters stringification no
longer exposes the secret.
In `@ct-app/core/components/peer.py`:
- Around line 62-73: The setter Peer.safe_address_count currently skips updating
Prometheus metrics when self.safe is None, hiding init-order bugs; change it to
fail fast instead: in safe_address_count.setter, after setting
self._safe_address_count, check that self.safe is not None and raise a clear
exception (e.g., ValueError or AssertionError) if it is, otherwise call
NODES_LINKED_TO_SAFE_COUNT.labels(self.address.native,
self.safe.address).set(value); reference the Peer.safe_address_count setter, the
safe attribute, and NODES_LINKED_TO_SAFE_COUNT.labels to locate the code to
change.
In `@ct-app/core/components/utils.py`:
- Around line 143-144: Remove the duplicate unreachable return of
keepalive_methods: inside the function that builds/returns the keepalive_methods
collection (the block where keepalive_methods is returned), delete the second
redundant "return keepalive_methods" so only a single return remains; ensure no
other logic was intended after the first return and run tests/linting to confirm
no leftover dead code (refer to the variable name keepalive_methods to locate
the exact spot).
In `@ct-app/core/mixins/state.py`:
- Around line 99-101: Guard the float conversion of ticket_price.value.value
used when setting the metric in TICKET_STATS.labels("price").set to avoid
raising on None or non-numeric strings: validate that ticket_price and its
nested value.value are present and convertible to float before calling
float(...), and if not, log a warning (including the offending value and
context) and skip updating the metric while still assigning self.ticket_price as
appropriate; ensure you handle exceptions (ValueError, TypeError) around
float(...) in the block that sets the metric so the coroutine does not crash.
🧹 Nitpick comments (7)
ct-app/core/subgraph/graphql_provider.py (1)
28-35: Type annotation improvements look good; minor edge-case consideration for__file__.The
query_file: str(without default) properly enforces that subclasses must define it.On line 34,
sys.modules[...].__file__can beNonein edge cases (frozen executables, certain import hooks). IfNone,str(None)produces"None", creating an invalid path. Consider adding a guard if this code may run in such environments:🛡️ Optional defensive fix
- self.pwd = Path(str(sys.modules[self.__class__.__module__].__file__)).parent + module_file = sys.modules[self.__class__.__module__].__file__ + if module_file is None: + raise RuntimeError(f"Cannot determine path for module {self.__class__.__module__}") + self.pwd = Path(module_file).parentct-app/core/api/hoprd_api.py (1)
258-297: Log exceptions before returning SessionFailure.The exception path returns a failure without logging; add
logger.exception(...)to keep stack traces and context for diagnostics.Suggested fix
except Exception as e: # Capture any exception details for better diagnostics error_type = type(e).__name__ error_msg = str(e) + logger.exception( + "Exception while opening session", + extra={ + "destination": destination, + "relayer": relayer, + "full_url": full_url, + }, + ) return resp.SessionFailure( { "error": f"{error_type}: {error_msg}",ct-app/.env.example (1)
5-5: Add a trailing newline at the end of the file.Most tools and POSIX standards expect files to end with a newline character.
Proposed fix
-SUBGRAPH_API_KEY='<SUBGRAPH_API_KEY>' +SUBGRAPH_API_KEY='<SUBGRAPH_API_KEY>' +ct-app/core/blokli/queries/health.graphql (1)
1-3: Inconsistent query syntax with other GraphQL files.This file uses
query()with empty parentheses, whileversion.graphqlusesquerywithout parentheses. For consistency and conventional GraphQL style, consider removing the empty parentheses.Proposed fix
-query() { +query { health }ct-app/test_blokli_query.py (1)
1-18: Clarify intent: Is this a committed test or a developer utility script?This file appears to be an exploratory/manual script rather than an automated test:
- No test framework (pytest/unittest) is used
- Hardcoded URL at line 7 limits portability across environments
- No assertions, only print statements
- Commented-out code suggests work in progress
If this is intended for CI/automated testing, consider:
- Using a test framework with proper assertions
- Parameterizing the URL via environment variable or config
- Adding error handling for network failures
If this is a developer utility, consider moving it to a
scripts/ortools/directory, or adding it to.gitignore.ct-app/core/components/peer.py (1)
46-55: Align channel_balance setter typing with Optional return type.The property now returns Optional[Balance] and the setter already tolerates None, but the parameter type still forbids it. This will confuse typing and callers.
♻️ Suggested typing fix
- def channel_balance(self, value: Balance): + def channel_balance(self, value: Optional[Balance]):ct-app/core/blokli/blokli_provider.py (1)
12-18: Avoid configuring logging at import time.Calling
configure_logging()on import mutates global logging for the whole process/tests. Prefer invoking it in the app entrypoint or guard it behind a one‑time init.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ct-app/core/components/peer.py (1)
76-83:⚠️ Potential issue | 🟡 MinorError message is misleading and line 82 check is unreachable.
- Line 77 raises "Safe balance not set" but it's checking if
safeitself is unset—consider "Safe not set" for clarity.- Line 82 check
if self.safe_address_count is Noneis dead code: thesafe_address_countproperty (lines 62-66) always returns anint(defaults to1whenNone), so this condition can never be true.🧹 Proposed fix
`@property` def split_stake(self) -> Balance: if not self.safe: - raise ValueError("Safe balance not set") + raise ValueError("Safe not set") if self.safe.balance is None: raise ValueError("Safe balance not set") if self.channel_balance is None: raise ValueError("Channel balance not set") - if self.safe_address_count is None: - raise ValueError("Safe address count not set") if self.yearly_message_count is None: return Balance.zero("wxHOPR") return self.safe.total_balance / self.safe_address_count + self.channel_balance
🤖 Fix all issues with AI agents
In `@ct-app/core/components/config_parser/base_classes.py`:
- Around line 75-103: In _coerce_value ensure Flag and Duration coercion return
consistent wrapper instances: in the Flag branch of _coerce_value (currently
testing isinstance(value, Flag)) return the Flag instance itself instead of
value.value, and in the non-instance path wrap raw values with Flag(value); do
the same for Duration (return the Duration instance when already one, otherwise
return Duration(value)), so both branches yield the same wrapper type and avoid
mixing raw values with wrapper instances.
In `@ct-app/core/subgraph/graphql_provider.py`:
- Around line 177-179: The except ProviderError handler in
GraphQLProvider._execute is dead code because _execute never raises
ProviderError (it returns ({}, None) on errors) — remove the except
ProviderError: block (including logger.exception("ProviderError error") and the
break) from the try/except in _execute (or alternatively, if you intend _execute
to raise on error, modify _execute to raise ProviderError instead of returning
({}, None)); update any related tests to reflect the chosen behavior and ensure
_get/_execute logic remains consistent.
In `@ct-app/test/test_config.yaml`:
- Around line 106-107: Replace the raw float values for the session retry delays
with explicit duration literals to match the new config format: change
session_retry_base_delay: 2.0 to a duration like session_retry_base_delay: "2s"
and session_retry_max_delay: 60.0 to session_retry_max_delay: "60s" so parsers
expecting duration strings (used elsewhere) will succeed; update any tests that
assert numeric types to expect strings if necessary.
🧹 Nitpick comments (2)
ct-app/core/subgraph/graphql_provider.py (2)
28-42: Consider initializing_sessionin__init__rather than as a class attribute.The
_sessionis declared as a class attribute (line 31) but is then assigned as an instance attribute in_execute. While this works because the instance attribute shadows the class attribute, it's cleaner to initialize mutable/stateful attributes in__init__to make the ownership clear.♻️ Suggested refactor
class GraphQLProvider: query_file: str params: list[str] = [] default_key: Optional[str] = None - _session: Optional[aiohttp.ClientSession] = None def __init__(self, url: URL): self.url = url self.pwd = Path(str(sys.modules[self.__class__.__module__].__file__)).parent self._initialize_query(self.query_file, self.params) self._timeout = aiohttp.ClientTimeout(total=30) + self._session: Optional[aiohttp.ClientSession] = None
69-72: Key extraction may behave unexpectedly if query body lacks parentheses.The expression
body.split("(")[0]assumes the query contains a(character. If the query file is malformed or uses a different format, this returns the entire body as the key, which may not be the intended behavior.Consider adding validation or a fallback:
🛡️ Suggested defensive handling
with open(self.pwd.joinpath(path)) as f: body = f.read() - return body.split("(")[0], ("\n".join([header, body, footer])) + key = body.split("(")[0].strip() if "(" in body else body.strip() + return key, ("\n".join([header, body, footer]))
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ct-app/core/__main__.py`:
- Around line 27-28: After calling params.blokli.set_attribute_from_env("url",
"BLOKLI_URL") and params.blokli.set_attribute_from_env("token", "BLOKLI_TOKEN")
add explicit validation that params.blokli.url and params.blokli.token are
non-empty (truthy); if either is empty raise a clear exception (e.g., ValueError
or RuntimeError) with a message mentioning the missing BLOKLI_URL or
BLOKLI_TOKEN so the startup fails fast and shows which env var is missing or
empty.
🧹 Nitpick comments (1)
ct-app/core/components/utils.py (1)
50-55: UpdateallowManyNodePerSafedocstring to match the newset[Peer]signature.The docstring still references a list and contains a typo (“equaly”). This can mislead users of the method. Consider updating the parameter description and wording.
💡 Suggested docstring fix
- Split the stake managed by a safe address equaly between the nodes + Split the stake managed by a safe address equally between the nodes that the safe manages. - :param peer: list of peers + :param peers: set of peers
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ct-app/.configs/core_staging_config.yaml`:
- Line 90: Replace the placeholder comments for the list-valued config keys with
explicit empty-list YAML syntax so they parse as lists instead of null;
specifically change the values for excluded_peers, green_destinations, and
blue_destinations from "# TODO" (null) to an explicit empty list ([]). This
ensures downstream code that does membership checks (state.py where membership
tests use "in") and list concatenation (economic_system.py where lists are
added) receive actual lists and avoid TypeError at runtime.
| minimum_delay_between_batches: 10s | ||
| sleep_mean_time: 10s | ||
| sleep_std_time: 2s | ||
| excluded_peers: # TODO |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.yaml" -o -name "*.yml" | head -20Repository: hoprnet/ct-research
Length of output: 359
🏁 Script executed:
fd -e yaml -e yml | grep -i configRepository: hoprnet/ct-research
Length of output: 91
🏁 Script executed:
cat -n ct-app/.configs/core_staging_config.yaml | sed -n '85,110p'Repository: hoprnet/ct-research
Length of output: 1083
🏁 Script executed:
rg -l "excluded_peers|green_destinations|blue_destinations" --type py -g "*.py"Repository: hoprnet/ct-research
Length of output: 222
🏁 Script executed:
rg -n "excluded_peers|green_destinations|blue_destinations" --type py -A 3 -B 3Repository: hoprnet/ct-research
Length of output: 2565
🏁 Script executed:
cat -n ct-app/.configs/core_prod_config.yaml | sed -n '85,110p'Repository: hoprnet/ct-research
Length of output: 1139
🏁 Script executed:
cat -n ct-app/.configs/core_local_config.yaml | sed -n '85,110p'Repository: hoprnet/ct-research
Length of output: 1111
🏁 Script executed:
cat -n ct-app/test/test_config.yaml | grep -A 5 -B 5 "excluded_peers\|green_destinations\|blue_destinations"Repository: hoprnet/ct-research
Length of output: 980
Replace # TODO comments with empty list syntax to prevent runtime TypeError.
In YAML, excluded_peers: # TODO parses as excluded_peers: null. The same applies to green_destinations and blue_destinations on lines 104–105. All other config files (prod, local, test) correctly use [] for these fields.
The Python code performs membership tests (in operator on lines 60, 62 of state.py) and list concatenation (line 45 of economic_system.py) on these fields. Passing None instead of an empty list will cause a TypeError at runtime:
if self.address.native in None→ TypeErrorNone + list→ TypeError
Proposed fix
- excluded_peers: # TODO
+ excluded_peers: []- green_destinations: # TODO
- blue_destinations: # TODO
+ green_destinations: []
+ blue_destinations: []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| excluded_peers: # TODO | |
| excluded_peers: [] |
| excluded_peers: # TODO | |
| green_destinations: [] | |
| blue_destinations: [] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ct-app/.configs/core_staging_config.yaml` at line 90, Replace the placeholder
comments for the list-valued config keys with explicit empty-list YAML syntax so
they parse as lists instead of null; specifically change the values for
excluded_peers, green_destinations, and blue_destinations from "# TODO" (null)
to an explicit empty list ([]). This ensures downstream code that does
membership checks (state.py where membership tests use "in") and list
concatenation (economic_system.py where lists are added) receive actual lists
and avoid TypeError at runtime.
1 participant
Summary by CodeRabbit
New Features
Chores