refactor: pure decision + effects architecture for consensus#1498
Merged
cristiam86 merged 4 commits intomainfrom Mar 10, 2026
Merged
refactor: pure decision + effects architecture for consensus#1498cristiam86 merged 4 commits intomainfrom
cristiam86 merged 4 commits intomainfrom
Conversation
…architecture Refactor the 9 consensus states (Pending, Proposing, Committing, Revealing, Accepted, Finalized, Undetermined, LeaderTimeout, ValidatorsTimeout) from impure mixed logic (decision + side effects) to a pure decision function + EffectExecutor pattern. Changes: - New effects.py: 24 frozen dataclasses describing all consensus side effects - New effect_executor.py: EffectExecutor class dispatches effects to infrastructure - New decisions.py: 20+ pure decision functions extracting testable logic from each state - Updated base.py: All state handle() methods now call decision functions + effects - New tests: 248+ unit tests for pure decision functions (no mocks needed) This enables full test coverage of decision logic without mocking infrastructure, separation of concerns, and easier auditing of consensus logic. All 248 unit tests passing. Worker unchanged.
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a pure, effect-driven consensus decision layer: new decision functions, immutable Effect dataclasses, an EffectExecutor to route effects to services, a TransitionResult type, and extensive unit tests validating decision and executor behavior across consensus flows and appeal scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant CA as ConsensusAlgorithm
participant DF as DecisionFunction
participant EX as EffectExecutor
participant TP as TransactionsProcessor
participant MH as MessageHandler
participant CS as ConsensusService
participant CP as ContractProcessor
CA->>DF: call decide_*(tx_hash, state_data, ...)
DF->>DF: compute next_state and effects[]
DF-->>CA: return effects[], next_state
CA->>EX: execute(effects)
loop for each Effect
EX->>EX: inspect effect type
alt AddTimestampEffect
EX->>TP: set_transaction_timestamp(tx_hash, state_name)
else StatusUpdateEffect
EX->>TP: update_status(tx_hash, new_status)
EX->>MH: dispatch LogEvent (async/sync)
else EmitRollupEventEffect
EX->>CS: emit_rollup_event(event_name, account, tx_hash, ...)
else RegisterContractEffect / UpdateContractStateEffect
EX->>CP: register_contract(...) / update_contract_state(...)
else DBWriteEffect / SetTransactionResultEffect / Appeal / Rotation / History Effects
EX->>TP: call corresponding method(...)
else Unknown
EX-->>EX: raise TypeError
end
end
EX-->>CA: execution complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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🧪 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: 5
🧹 Nitpick comments (5)
backend/consensus/transition.py (1)
23-24:TransitionResultis only shallowly immutable right now.
frozen=Truedoes not prevent mutatinglist/dictcontents, so transition payloads can still be changed after creation.♻️ Suggested refactor
from dataclasses import dataclass, field +from types import MappingProxyType -from typing import Any +from typing import Any, Mapping @@ - effects: list[Effect] = field(default_factory=list) - context_updates: dict[str, Any] = field(default_factory=dict) + effects: tuple[Effect, ...] = () + context_updates: Mapping[str, Any] = field( + default_factory=lambda: MappingProxyType({}) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/consensus/transition.py` around lines 23 - 24, TransitionResult is frozen but still shallowly mutable because effects is a list and context_updates is a dict; convert them to immutable types on construction (e.g., change effects to a tuple[Effect, ...] and context_updates to an immutable mapping like types.MappingProxyType or Mapping[str, Any]) or implement a __post_init__ in the TransitionResult dataclass to replace list/dict inputs with tuple(...) and a MappingProxyType(...) copy so callers cannot mutate contents after creation; mention the class name TransitionResult and the field names effects and context_updates when making the change.backend/consensus/effect_executor.py (1)
41-48: Type the executor context interface.Using an explicit context protocol here will catch missing dependencies at type-check time.
♻️ Proposed fix
+from typing import Any, Protocol + +class EffectExecutorContext(Protocol): + transactions_processor: Any + msg_handler: Any + consensus_service: Any + contract_processor: Any + class EffectExecutor: @@ - def __init__(self, context): + def __init__(self, context: EffectExecutorContext) -> None: """ Args: context: A TransactionContext (or any object exposing transactions_processor, msg_handler, consensus_service, contract_processor). """ self.ctx = contextAs per coding guidelines, "Include type hints in all Python code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/consensus/effect_executor.py` around lines 41 - 48, Define a typed context Protocol and use it in EffectExecutor.__init__: create a Protocol named TransactionContext (or ExecutorContext) that declares the required attributes transactions_processor, msg_handler, consensus_service, and contract_processor with appropriate types or Any (import Protocol, Any from typing); update the __init__ signature to def __init__(self, context: TransactionContext) -> None and keep self.ctx = context so missing attributes are caught by type checkers. Ensure the Protocol is imported/defined in the same module (or imported) so mypy/pyright can validate usages.backend/consensus/decisions.py (2)
768-777: Prune or underscore unused parameters indecide_post_proposal.
leader_receipt_result,leader_address, andremaining_validatorsare not read and currently create avoidable API noise.♻️ Proposed fix
def decide_post_proposal( tx_hash: str, - leader_receipt_result: bytes, + _leader_receipt_result: bytes, leader_receipt_timed_out: bool, execution_mode_leader_only: bool, appeal_leader_timeout: bool, - leader_address: str, + _leader_address: str, leader: dict, - remaining_validators: list[dict], + _remaining_validators: list[dict], consensus_data_dict: dict, ) -> tuple[str, list[Effect]]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/consensus/decisions.py` around lines 768 - 777, decide_post_proposal currently declares unused parameters leader_receipt_result, leader_address, and remaining_validators; either remove them from the function signature (and update all callers to stop passing those args) or mark them as intentionally unused by prefixing with an underscore (_leader_receipt_result, _leader_address, _remaining_validators) and update the type hints and any docstring to reflect this; ensure you modify the decide_post_proposal definition and any direct callers to match the chosen approach so there are no unused-parameter lint errors or API noise.
707-723: Trim unused inputs indecide_pending_preto keep the decision API tight.
appeal_leader_timeoutandappeal_undeterminedare currently unused in this function.♻️ Proposed fix
def decide_pending_pre( tx_hash: str, - appeal_leader_timeout: bool, - appeal_undetermined: bool, + _appeal_leader_timeout: bool, + _appeal_undetermined: bool, ) -> list[Effect]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/consensus/decisions.py` around lines 707 - 723, decide_pending_pre currently accepts unused parameters appeal_leader_timeout and appeal_undetermined; remove these unused arguments from the function signature (keep decide_pending_pre(tx_hash: str) -> list[Effect]) and update any callers to stop passing those two booleans (or adapt their call sites to pass only tx_hash). Ensure the type hint and docstring remain accurate and that imports/returns (AddTimestampEffect, ResetRotationCountEffect, ResetRotationCountEffect import) are left intact; run tests/linters to catch any remaining references to the old signature.tests/unit/consensus/test_decisions_accepted.py (1)
34-92: Add type hints to test helpers and kwargs builders.These local utilities are part of the changed surface and are currently untyped.
♻️ Proposed fix
+from typing import Any, TypeVar + +T = TypeVar("T") + -def _effect_types(effects): +def _effect_types(effects: list[object]) -> list[str]: """Return a list of effect class names for quick structural checks.""" return [type(e).__name__ for e in effects] -def _find_effect(effects, effect_type): +def _find_effect(effects: list[object], effect_type: type[T]) -> T | None: """Find the first effect of a given type.""" for e in effects: if isinstance(e, effect_type): return e return None -def _find_effects(effects, effect_type): +def _find_effects(effects: list[object], effect_type: type[T]) -> list[T]: """Find all effects of a given type.""" return [e for e in effects if isinstance(e, effect_type)] -def _base_accepted_kwargs(**overrides): +def _base_accepted_kwargs(**overrides: Any) -> dict[str, Any]: """Build default kwargs for decide_accepted, override as needed.""" defaults = dict( ... ) defaults.update(overrides) return defaults -def _base_finalizing_kwargs(**overrides): +def _base_finalizing_kwargs(**overrides: Any) -> dict[str, Any]: """Build default kwargs for decide_finalizing, override as needed.""" defaults = dict( ... ) defaults.update(overrides) return defaultsAs per coding guidelines, "Include type hints in all Python code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/consensus/test_decisions_accepted.py` around lines 34 - 92, Add explicit type hints to the test helper functions: annotate _effect_types(effects) to accept Iterable[Any] (or Iterable[Effect]) and return List[str]; _find_effect(effects, effect_type) to accept Iterable[Any] and effect_type: Type[T] and return Optional[T] (use a TypeVar T); _find_effects(effects, effect_type) to accept Iterable[Any] and effect_type: Type[T] and return List[T]; _base_accepted_kwargs(**overrides) and _base_finalizing_kwargs(**overrides) to return Dict[str, Any] and accept **overrides: Any. Also add the necessary typing imports (List, Optional, Type, TypeVar, Iterable, Dict, Any) at the top of the file and use the TypeVar T to express the generic effect type.
🤖 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/consensus/decisions.py`:
- Around line 164-169: In the branch that sets consensus_round =
ConsensusRound.LEADER_APPEAL_SUCCESSFUL you must also clear the lingering
appeal_undetermined flag to avoid leaking state; update the effects list in that
branch (alongside SetTimestampAwaitingFinalizationEffect,
ResetAppealProcessingTimeEffect, SetTimestampAppealEffect) to include an effect
that clears the undetermined marker (e.g., append a
ClearAppealUndeterminedEffect or a SetAppealUndeterminedEffect with the cleared
value, passing tx_hash=tx_hash) so subsequent decision logic no longer sees
stale appeal_undetermined state.
In `@backend/consensus/effects.py`:
- Around line 37-214: Several frozen dataclasses (e.g., SendMessageEffect,
EmitRollupEventEffect, DBWriteEffect, RegisterContractEffect,
UpdateContractStateEffect, InsertTriggeredTransactionEffect,
UpdateConsensusHistoryEffect, SetTransactionResultEffect,
SetContractSnapshotEffect, SetLeaderTimeoutValidatorsEffect, etc.) expose
mutable nested fields (dict/list) that can still be mutated; change those field
types to immutable interfaces (Mapping, Sequence, tuple) and/or implement a
__post_init__ on each affected dataclass (e.g., DBWriteEffect.__post_init__,
InsertTriggeredTransactionEffect.__post_init__,
UpdateConsensusHistoryEffect.__post_init__) that performs defensive deep copies
and wraps mappings with MappingProxyType and sequences as tuples (using deepcopy
for nested structures) so the frozen dataclass truly prevents nested mutation,
and update type annotations accordingly.
In `@tests/unit/consensus/test_decisions_accepted.py`:
- Line 103: The test currently unpacks decide_accepted(...) into pre, post, cr,
rv but pre, post and rv are unused; update the unpacking so the unused values
are assigned to underscores and only cr remains a real variable (i.e., discard
pre, post, rv by replacing them with _ in the left-hand unpack) in the
decide_accepted call to silence RUF059.
In `@tests/unit/consensus/test_decisions_terminal.py`:
- Line 79: The test unpacks a tuple from decide_undetermined into a variable
named effects that is never used; update the unpacking in the test calls to
discard the first value (use _ instead of effects) wherever
decide_undetermined(**self._base_kwargs()) is assigned (e.g., the occurrences at
the same pattern around lines shown) so the unused variable warning (RUF059) is
resolved while leaving the returned round_ binding intact.
In `@tests/unit/consensus/test_effect_executor.py`:
- Around line 485-490: The two side-effect lambdas for
ctx.transactions_processor.add_state_timestamp and
ctx.transactions_processor.update_transaction_status use an unused parameter
name "*a" which triggers ARG005; change them to use "*_" (e.g., lambda *_:
call_order.append("timestamp") and lambda *_: call_order.append("status")) so
the unused var is explicit and Ruff is satisfied while preserving behavior
referencing call_order.
---
Nitpick comments:
In `@backend/consensus/decisions.py`:
- Around line 768-777: decide_post_proposal currently declares unused parameters
leader_receipt_result, leader_address, and remaining_validators; either remove
them from the function signature (and update all callers to stop passing those
args) or mark them as intentionally unused by prefixing with an underscore
(_leader_receipt_result, _leader_address, _remaining_validators) and update the
type hints and any docstring to reflect this; ensure you modify the
decide_post_proposal definition and any direct callers to match the chosen
approach so there are no unused-parameter lint errors or API noise.
- Around line 707-723: decide_pending_pre currently accepts unused parameters
appeal_leader_timeout and appeal_undetermined; remove these unused arguments
from the function signature (keep decide_pending_pre(tx_hash: str) ->
list[Effect]) and update any callers to stop passing those two booleans (or
adapt their call sites to pass only tx_hash). Ensure the type hint and docstring
remain accurate and that imports/returns (AddTimestampEffect,
ResetRotationCountEffect, ResetRotationCountEffect import) are left intact; run
tests/linters to catch any remaining references to the old signature.
In `@backend/consensus/effect_executor.py`:
- Around line 41-48: Define a typed context Protocol and use it in
EffectExecutor.__init__: create a Protocol named TransactionContext (or
ExecutorContext) that declares the required attributes transactions_processor,
msg_handler, consensus_service, and contract_processor with appropriate types or
Any (import Protocol, Any from typing); update the __init__ signature to def
__init__(self, context: TransactionContext) -> None and keep self.ctx = context
so missing attributes are caught by type checkers. Ensure the Protocol is
imported/defined in the same module (or imported) so mypy/pyright can validate
usages.
In `@backend/consensus/transition.py`:
- Around line 23-24: TransitionResult is frozen but still shallowly mutable
because effects is a list and context_updates is a dict; convert them to
immutable types on construction (e.g., change effects to a tuple[Effect, ...]
and context_updates to an immutable mapping like types.MappingProxyType or
Mapping[str, Any]) or implement a __post_init__ in the TransitionResult
dataclass to replace list/dict inputs with tuple(...) and a
MappingProxyType(...) copy so callers cannot mutate contents after creation;
mention the class name TransitionResult and the field names effects and
context_updates when making the change.
In `@tests/unit/consensus/test_decisions_accepted.py`:
- Around line 34-92: Add explicit type hints to the test helper functions:
annotate _effect_types(effects) to accept Iterable[Any] (or Iterable[Effect])
and return List[str]; _find_effect(effects, effect_type) to accept Iterable[Any]
and effect_type: Type[T] and return Optional[T] (use a TypeVar T);
_find_effects(effects, effect_type) to accept Iterable[Any] and effect_type:
Type[T] and return List[T]; _base_accepted_kwargs(**overrides) and
_base_finalizing_kwargs(**overrides) to return Dict[str, Any] and accept
**overrides: Any. Also add the necessary typing imports (List, Optional, Type,
TypeVar, Iterable, Dict, Any) at the top of the file and use the TypeVar T to
express the generic effect type.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/consensus/base.pybackend/consensus/decisions.pybackend/consensus/effect_executor.pybackend/consensus/effects.pybackend/consensus/transition.pytests/unit/consensus/test_base.pytests/unit/consensus/test_contract_state_stripping.pytests/unit/consensus/test_decisions_accepted.pytests/unit/consensus/test_decisions_caller_helpers.pytests/unit/consensus/test_decisions_pending_proposing_committing.pytests/unit/consensus/test_decisions_revealing.pytests/unit/consensus/test_decisions_terminal.pytests/unit/consensus/test_effect_executor.pytests/unit/consensus/test_effects.pytests/unit/consensus/test_helpers.pytests/unit/consensus/test_validator_exec_timeout.py
💤 Files with no reviewable changes (2)
- tests/unit/consensus/test_contract_state_stripping.py
- tests/unit/consensus/test_helpers.py
| """Normal acceptance path: not appealed, not appeal_undetermined.""" | ||
|
|
||
| def test_consensus_round_is_accepted(self): | ||
| pre, post, cr, rv = decide_accepted(**_base_accepted_kwargs()) |
Contributor
There was a problem hiding this comment.
Silence RUF059 by discarding unused unpacked values.
pre, post, and rv are intentionally unused here; unpack them to _.
♻️ Proposed fix
- pre, post, cr, rv = decide_accepted(**_base_accepted_kwargs())
+ _, _, cr, _ = decide_accepted(**_base_accepted_kwargs())📝 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.
Suggested change
| pre, post, cr, rv = decide_accepted(**_base_accepted_kwargs()) | |
| _, _, cr, _ = decide_accepted(**_base_accepted_kwargs()) |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 103-103: Unpacked variable pre is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 103-103: Unpacked variable post is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 103-103: Unpacked variable rv is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/consensus/test_decisions_accepted.py` at line 103, The test
currently unpacks decide_accepted(...) into pre, post, cr, rv but pre, post and
rv are unused; update the unpacking so the unused values are assigned to
underscores and only cr remains a real variable (i.e., discard pre, post, rv by
replacing them with _ in the left-hand unpack) in the decide_accepted call to
silence RUF059.
| return defaults | ||
|
|
||
| def test_normal_undetermined_round(self): | ||
| effects, round_ = decide_undetermined(**self._base_kwargs()) |
Contributor
There was a problem hiding this comment.
Fix Ruff RUF059 by discarding unused unpacked values.
effects is unpacked but never used in these tests; replace it with _ to keep lint green.
🧹 Suggested patch
- effects, round_ = decide_undetermined(**self._base_kwargs())
+ _, round_ = decide_undetermined(**self._base_kwargs())
- effects, round_ = decide_undetermined(
+ _, round_ = decide_undetermined(
**self._base_kwargs(appeal_undetermined=True, appeal_failed=1)
)
- effects, round_ = decide_leader_timeout(**self._base_kwargs())
+ _, round_ = decide_leader_timeout(**self._base_kwargs())
- effects, round_ = decide_leader_timeout(
+ _, round_ = decide_leader_timeout(
**self._base_kwargs(appeal_undetermined=True)
)
- effects, round_ = decide_leader_timeout(
+ _, round_ = decide_leader_timeout(
**self._base_kwargs(appeal_leader_timeout=True)
)
- effects, round_ = decide_validators_timeout(**self._base_kwargs())
+ _, round_ = decide_validators_timeout(**self._base_kwargs())
- effects, round_ = decide_validators_timeout(
+ _, round_ = decide_validators_timeout(
**self._base_kwargs(appeal_undetermined=True)
)
- effects, round_ = decide_validators_timeout(
+ _, round_ = decide_validators_timeout(
**self._base_kwargs(appeal_validators_timeout=True, appeal_failed=1)
)Also applies to: 148-148, 202-202, 257-257, 285-285, 330-330, 374-374, 410-410
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 79-79: Unpacked variable effects is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/consensus/test_decisions_terminal.py` at line 79, The test unpacks
a tuple from decide_undetermined into a variable named effects that is never
used; update the unpacking in the test calls to discard the first value (use _
instead of effects) wherever decide_undetermined(**self._base_kwargs()) is
assigned (e.g., the occurrences at the same pattern around lines shown) so the
unused variable warning (RUF059) is resolved while leaving the returned round_
binding intact.
Comment on lines
+485
to
+490
| ctx.transactions_processor.add_state_timestamp.side_effect = ( | ||
| lambda *a: call_order.append("timestamp") | ||
| ) | ||
| ctx.transactions_processor.update_transaction_status.side_effect = ( | ||
| lambda *a: call_order.append("status") | ||
| ) |
Contributor
There was a problem hiding this comment.
Address ARG005 in side-effect lambdas.
The lambda parameters are unused; use *_ to satisfy Ruff.
🧹 Suggested patch
- ctx.transactions_processor.add_state_timestamp.side_effect = (
- lambda *a: call_order.append("timestamp")
- )
+ ctx.transactions_processor.add_state_timestamp.side_effect = (
+ lambda *_: call_order.append("timestamp")
+ )
- ctx.transactions_processor.update_transaction_status.side_effect = (
- lambda *a: call_order.append("status")
- )
+ ctx.transactions_processor.update_transaction_status.side_effect = (
+ lambda *_: call_order.append("status")
+ )📝 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.
Suggested change
| ctx.transactions_processor.add_state_timestamp.side_effect = ( | |
| lambda *a: call_order.append("timestamp") | |
| ) | |
| ctx.transactions_processor.update_transaction_status.side_effect = ( | |
| lambda *a: call_order.append("status") | |
| ) | |
| ctx.transactions_processor.add_state_timestamp.side_effect = ( | |
| lambda *_: call_order.append("timestamp") | |
| ) | |
| ctx.transactions_processor.update_transaction_status.side_effect = ( | |
| lambda *_: call_order.append("status") | |
| ) |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 486-486: Unused lambda argument: a
(ARG005)
[warning] 489-489: Unused lambda argument: a
(ARG005)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/consensus/test_effect_executor.py` around lines 485 - 490, The two
side-effect lambdas for ctx.transactions_processor.add_state_timestamp and
ctx.transactions_processor.update_transaction_status use an unused parameter
name "*a" which triggers ARG005; change them to use "*_" (e.g., lambda *_:
call_order.append("timestamp") and lambda *_: call_order.append("status")) so
the unused var is explicit and Ruff is satisfied while preserving behavior
referencing call_order.
…ability Frozen dataclasses only prevent attribute rebinding, not nested mutation of dict/list fields. The base Effect.__post_init__ now deepcopies all mutable containers on construction so callers cannot mutate effect payloads through shared references. Addresses CodeRabbit review feedback on PR #1498.
The validators_timeout path already cleared this flag but the leader_timeout path did not — a pre-existing inconsistency surfaced by CodeRabbit review on PR #1498.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
backend/consensus/decisions.py (3)
546-554: Consider adding comments explaining the validator slicing formula.The slicing logic with
n - 1in lines 549-550 and 552-553 is non-obvious. For edge cases (e.g., smallexisting_validatorslists),ncan be negative, resulting in slices like[:-2]. While this is safe Python behavior, the intent behind these formulas isn't clear from the code alone.Consider adding brief comments explaining the expected validator counts per appeal round.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/consensus/decisions.py` around lines 546 - 554, The slicing with n - 1 in the appeal handling logic (variables: appeal_failed, merged_validators, existing_validators, current_validation_results) is non-obvious and can produce negative indices; add a brief inline comment above each branch explaining the intended validator counts and why n is computed that way (e.g., how many validators to keep from existing_validators for appeal_failed == 1 and the fallback branch), and mention that negative n results in full-list behavior due to Python slicing semantics or clamp n to 0 if that is the intended safe behavior; this will make the intent explicit and clarify edge-case handling for small existing_validators lists.
701-702: Minor: Extract exception message to class or use brief inline message.Ruff TRY003 flags the long message. Since this is an internal invariant violation (enum exhaustion), a brief message suffices.
- raise ValueError("Invalid consensus result") + raise ValueError(f"unexpected ConsensusResult: {consensus_result}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/consensus/decisions.py` around lines 701 - 702, The raise in decisions.py uses a long inline message; define a short module-level custom exception (e.g., class ConsensusInvariantError(ValueError): pass) and replace the raise ValueError("Invalid consensus result") with raise ConsensusInvariantError so the invariant is signaled without a long string; add the new class near other exceptions in the module and update any imports/uses accordingly.
769-831: Remove unused function parameters.
leader_receipt_result,leader_address, andremaining_validatorsare declared but never used in the function body. Remove them to reduce maintenance burden and avoid future confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/consensus/decisions.py` around lines 769 - 831, The function decide_post_proposal declares unused parameters (leader_receipt_result, leader_address, remaining_validators); remove these parameters from the decide_post_proposal signature and its type hints, delete any related references in the docstring/Args list, and keep the body unchanged (retain existing effects logic). After changing the signature, update all call sites to stop passing those three arguments (search for decide_post_proposal(...) usages) so callers pass only the remaining parameters in the new order, and run type checks/tests to ensure no remaining references to those names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/consensus/decisions.py`:
- Around line 546-554: The slicing with n - 1 in the appeal handling logic
(variables: appeal_failed, merged_validators, existing_validators,
current_validation_results) is non-obvious and can produce negative indices; add
a brief inline comment above each branch explaining the intended validator
counts and why n is computed that way (e.g., how many validators to keep from
existing_validators for appeal_failed == 1 and the fallback branch), and mention
that negative n results in full-list behavior due to Python slicing semantics or
clamp n to 0 if that is the intended safe behavior; this will make the intent
explicit and clarify edge-case handling for small existing_validators lists.
- Around line 701-702: The raise in decisions.py uses a long inline message;
define a short module-level custom exception (e.g., class
ConsensusInvariantError(ValueError): pass) and replace the raise
ValueError("Invalid consensus result") with raise ConsensusInvariantError so the
invariant is signaled without a long string; add the new class near other
exceptions in the module and update any imports/uses accordingly.
- Around line 769-831: The function decide_post_proposal declares unused
parameters (leader_receipt_result, leader_address, remaining_validators); remove
these parameters from the decide_post_proposal signature and its type hints,
delete any related references in the docstring/Args list, and keep the body
unchanged (retain existing effects logic). After changing the signature, update
all call sites to stop passing those three arguments (search for
decide_post_proposal(...) usages) so callers pass only the remaining parameters
in the new order, and run type checks/tests to ensure no remaining references to
those names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28f7c39e-48b7-4c1b-9550-ecff4c64cced
📒 Files selected for processing (2)
backend/consensus/decisions.pytests/unit/consensus/test_decisions_terminal.py
Incorporates main's removal of the redundant 'code' key from contract data (PR #1481). Conflicts resolved: - base.py: keep EffectExecutor approach, remove contract_code param - decisions.py: remove contract_code param and "code" key from contract dict - test_helpers.py: keep our deletion of setup_test_environment/cleanup_threads - test_base.py, test_contract_state_stripping.py: keep deleted
Contributor
|
🎉 This PR is included in version 0.109.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
7 tasks
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.
What
Refactor the 9 consensus states from impure mixed logic (decision + side effects) to a pure decision function + EffectExecutor pattern.
Key Changes
handle()methods now call decision functions + effectsWhy
This architecture enables:
Testing Done
UpdateConsensusHistoryEffectto convertnew_statusstrings toTransactionStatusenumtest_validator_exec_timeout.pymock to includecontract_processorDecisions Made
handle()because it produces data needed for same-state decisionshandle()to avoid effect complexityChecks
Note: Integration tests require Docker rebuild to pick up the
TransactionStatusenum fix.Summary by CodeRabbit
Refactor
Tests