refactor: add TypedDicts for untyped dict parameters in backend#1592
Open
giwaov wants to merge 1 commit intogenlayerlabs:mainfrom
Open
refactor: add TypedDicts for untyped dict parameters in backend#1592giwaov wants to merge 1 commit intogenlayerlabs:mainfrom
giwaov wants to merge 1 commit intogenlayerlabs:mainfrom
Conversation
- Add LLMProviderConfig TypedDict for provider config (temperature, max_tokens, use_max_completion_tokens) - Add PluginConfig TypedDict for plugin config (api_key_env_var, api_url, mock_response) - Add ProviderParams TypedDict for add/update provider RPC params - Add NodeConfig TypedDict for Receipt.node_config validator dict - Update LLMProvider, SimValidatorConfig to use typed config dicts - Update add_provider, update_provider, create_validator, update_validator signatures - Update Receipt.node_config, consensus decisions to use NodeConfig Closes genlayerlabs#308
Contributor
📝 WalkthroughWalkthroughIntroduces typed dictionary structures for node and LLM provider configurations, replacing generic Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🧹 Nitpick comments (3)
backend/domain/types.py (1)
27-32: Consider makingProviderParamsfields optional withNotRequired.
ProviderParamsusestotal=True(the default), making all fields required. However,NotRequiredis imported on line 9 but not used. If some fields can be optional in certain RPC calls (e.g., when using defaults), consider usingNotRequiredfor consistency with howconfigandplugin_configcan beNonein validators.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/domain/types.py` around lines 27 - 32, ProviderParams currently makes all fields required; update it to mark fields that can be omitted using NotRequired (which is already imported) so RPC/validator paths that accept missing values work correctly: change the TypedDict declaration for ProviderParams to use NotRequired for any of provider, model, config, plugin, plugin_config that may be absent (and keep config/plugin_config types as Optional or allow None in validators as needed). Locate the ProviderParams TypedDict in backend/domain/types.py and update the field annotations to NotRequired[...] for the optional ones to reflect actual usage in your validators and RPC handlers.backend/node/types.py (2)
3-3: Unused import:NotRequiredis not used.With
total=Falseon the TypedDict, all fields are already optional. TheNotRequiredimport is unnecessary unless you plan to switch tototal=Truewith selectively optional fields.🧹 Remove unused import
-from typing import Iterable, Optional, Literal, NotRequired, TypedDict +from typing import Iterable, Optional, Literal, TypedDict🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/node/types.py` at line 3, The import list in backend/node/types.py includes an unused symbol NotRequired; remove NotRequired from the typing import (leave Iterable, Optional, Literal, TypedDict) so the unused import is eliminated and linting errors go away; if you intended to use NotRequired later, instead switch the TypedDict usage to total=True and apply NotRequired to specific keys, otherwise simply delete NotRequired from the import line.
229-242: Consider using the domain-level TypedDicts for nested config fields.The PR introduces
LLMProviderConfigandPluginConfiginbackend/domain/types.py. For improved type safety on nested fields, consider referencing those types here instead of genericdict:config: LLMProviderConfig # instead of dict plugin_config: PluginConfig # instead of dictHowever, if
primary_modelandsecondary_modelhave a different structure than the domain types, keeping them asdict | Noneis acceptable for now.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/node/types.py` around lines 229 - 242, The NodeConfig TypedDict currently uses generic dict for the nested fields; update the NodeConfig definition so that the config field uses the domain-level LLMProviderConfig type and plugin_config uses PluginConfig (importing them from backend/domain/types.py) instead of plain dict to improve type safety while leaving primary_model and secondary_model as dict | None if their shapes differ from domain types; ensure you update the imports and any mypy/typing references that expect these fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/domain/types.py`:
- Around line 21-24: The PluginConfig TypedDict is missing the mock_web_response
key used elsewhere; update the PluginConfig definition to include a
mock_web_response field (same shape as other mock fields, e.g.,
mock_web_response: dict) so type checkers recognize accesses like
plugin_config["mock_web_response"] and avoid warnings; modify the PluginConfig
class in types.py to add this field.
---
Nitpick comments:
In `@backend/domain/types.py`:
- Around line 27-32: ProviderParams currently makes all fields required; update
it to mark fields that can be omitted using NotRequired (which is already
imported) so RPC/validator paths that accept missing values work correctly:
change the TypedDict declaration for ProviderParams to use NotRequired for any
of provider, model, config, plugin, plugin_config that may be absent (and keep
config/plugin_config types as Optional or allow None in validators as needed).
Locate the ProviderParams TypedDict in backend/domain/types.py and update the
field annotations to NotRequired[...] for the optional ones to reflect actual
usage in your validators and RPC handlers.
In `@backend/node/types.py`:
- Line 3: The import list in backend/node/types.py includes an unused symbol
NotRequired; remove NotRequired from the typing import (leave Iterable,
Optional, Literal, TypedDict) so the unused import is eliminated and linting
errors go away; if you intended to use NotRequired later, instead switch the
TypedDict usage to total=True and apply NotRequired to specific keys, otherwise
simply delete NotRequired from the import line.
- Around line 229-242: The NodeConfig TypedDict currently uses generic dict for
the nested fields; update the NodeConfig definition so that the config field
uses the domain-level LLMProviderConfig type and plugin_config uses PluginConfig
(importing them from backend/domain/types.py) instead of plain dict to improve
type safety while leaving primary_model and secondary_model as dict | None if
their shapes differ from domain types; ensure you update the imports and any
mypy/typing references that expect these fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fd95247-53af-4f11-9cc3-db9e05e39386
📒 Files selected for processing (6)
backend/consensus/decisions.pybackend/domain/types.pybackend/node/base.pybackend/node/types.pybackend/protocol_rpc/endpoints.pybackend/protocol_rpc/rpc_methods.py
Comment on lines
+21
to
+24
| class PluginConfig(TypedDict, total=False): | ||
| api_key_env_var: str | ||
| api_url: str | ||
| mock_response: dict |
Contributor
There was a problem hiding this comment.
Missing mock_web_response field in PluginConfig.
Based on the relevant code snippet from backend/validators/__init__.py:203-215, the mock_web_response key is actively read from plugin_config:
if (
"mock_web_response" in val.llmprovider.plugin_config
and len(val.llmprovider.plugin_config["mock_web_response"]) > 0
):This field should be added to PluginConfig to ensure type consistency and avoid type checker warnings.
Proposed fix
class PluginConfig(TypedDict, total=False):
api_key_env_var: str
api_url: str
mock_response: dict
+ mock_web_response: dict🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/domain/types.py` around lines 21 - 24, The PluginConfig TypedDict is
missing the mock_web_response key used elsewhere; update the PluginConfig
definition to include a mock_web_response field (same shape as other mock
fields, e.g., mock_web_response: dict) so type checkers recognize accesses like
plugin_config["mock_web_response"] and avoid warnings; modify the PluginConfig
class in types.py to add this field.
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
Adds TypedDict definitions for previously untyped
dictparameters across the backend, improving static type checking and IDE support.Changes
New TypedDicts in
backend/domain/types.pyLLMProviderConfig- typed config for LLM providers (temperature,max_tokens,use_max_completion_tokens)PluginConfig- typed config for plugins (api_key_env_var,api_url,mock_response)ProviderParams- typed params for add/update provider RPC endpointsNew TypedDict in
backend/node/types.pyNodeConfig- typed validator node config used inReceipt.node_config(supports both flat validator dict and enhanced format withprimary_model/secondary_model)Updated signatures
LLMProvider.config/plugin_confignow useLLMProviderConfig/PluginConfigSimValidatorConfig.config/plugin_confignow useLLMProviderConfig/PluginConfigadd_provider/update_providerparams now useProviderParamscreate_validator/update_validatorconfig params now useLLMProviderConfig/PluginConfigReceipt.node_confignow usesNodeConfigdecide_accepted/decide_finalizingleader_node_confignow usesNodeConfigNode._create_enhanced_node_configreturn type now usesNodeConfigMotivation
As noted in the issue, ~90% of bugs have been due to incorrect field access on untyped dicts. These TypedDicts enable static analysis tools (Pylance, mypy) to catch field access errors at development time.
Closes #308
Summary by CodeRabbit