Conversation
📝 WalkthroughWalkthroughDefers matching: short-term orders placed during CheckTx use PlaceOrderNoMatch and are queued; matching runs later during PrepareProposal via MatchAllCrossedOrders, which replays queued takers to populate operations for proposal construction. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant CheckTx as CheckTx
participant Keeper as Keeper
participant MemClob as MemClob
participant Prepare as PrepareProposal
Client->>CheckTx: Submit order tx
CheckTx->>Keeper: PlaceShortTermOrder(tx)
Keeper->>MemClob: PlaceOrderNoMatch(order)
MemClob->>MemClob: Add to pendingMatchOrders
MemClob-->>Keeper: OrderStatus (deferred)
Keeper-->>CheckTx: CheckTx success
Note over CheckTx,Prepare: Later in same block (proposal construction)
Prepare->>Keeper: PrepareProposal
Prepare->>Keeper: MatchAllCrossedOrders(ctx)
Keeper->>MemClob: MatchAllCrossedOrders()
MemClob->>MemClob: Remove pending orders, replay as takers (arrival order)
MemClob->>MemClob: Execute matching, update book & tx bytes
MemClob-->>Keeper: Matching results / Operations
Keeper-->>Prepare: Operations populated for proposal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
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 @.github/workflows/protocol-build-and-push-snapshot.yml:
- Line 9: Remove the temporary feature-branch trigger 'kefan/short-term-cancel'
from the workflow push triggers so main will no longer run snapshot jobs for
that personal branch; locate the push: branches: list in the GitHub Actions
workflow (the entry containing 'kefan/short-term-cancel') and delete that string
from the array so only intended branches (e.g., main, release/*, etc.) remain.
| - main | ||
| - 'release/protocol/v[0-9]+.[0-9]+.x' # e.g. release/protocol/v0.1.x | ||
| - 'release/protocol/v[0-9]+.x' # e.g. release/protocol/v1.x | ||
| - 'kefan/short-term-cancel' |
There was a problem hiding this comment.
Remove this feature-branch trigger before merging to main.
Adding a personal/feature branch (kefan/short-term-cancel) to this workflow's push triggers means every commit to that branch fires all four snapshot jobs in parallel — including the build-and-push-snapshot-staging job. This pushes unreviewed feature-branch images directly to the staging ECR environment on every push, which is both an operational risk (staging receives untested builds) and wasteful (4× build minutes per commit).
This trigger is clearly a temporary developer convenience and must be removed before this PR is merged to main, where it would be permanently embedded in the mainline workflow.
🛠️ Proposed fix
branches:
- main
- 'release/protocol/v[0-9]+.[0-9]+.x' # e.g. release/protocol/v0.1.x
- 'release/protocol/v[0-9]+.x' # e.g. release/protocol/v1.x
- - 'kefan/short-term-cancel'📝 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.
| - 'kefan/short-term-cancel' | |
| branches: | |
| - main | |
| - 'release/protocol/v[0-9]+.[0-9]+.x' # e.g. release/protocol/v0.1.x | |
| - 'release/protocol/v[0-9]+.x' # e.g. release/protocol/v1.x |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/protocol-build-and-push-snapshot.yml at line 9, Remove the
temporary feature-branch trigger 'kefan/short-term-cancel' from the workflow
push triggers so main will no longer run snapshot jobs for that personal branch;
locate the push: branches: list in the GitHub Actions workflow (the entry
containing 'kefan/short-term-cancel') and delete that string from the array so
only intended branches (e.g., main, release/*, etc.) remain.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
protocol/testing/testnet-dev/dev.sh (1)
232-233: This addition violates the explicit "DO NOT add more config modifications" directive onedit_config().Lines 223–225 state:
# Note: DO NOT add more config modifications in this method. Use cmd/config.go to configure the default config values.Since
timeout_commit = 10sis dev-env-specific (not appropriate forcmd/config.goas a global default), the right approach here is to either:
- Apply the override in the calling site (
create_validators/create_full_nodes) rather than insideedit_config, or- Acknowledge the exception by updating the directive comment so future contributors aren't misled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/testing/testnet-dev/dev.sh` around lines 232 - 233, The new dasel change to set '.consensus.timeout_commit' to '10s' violates the "DO NOT add more config modifications" rule in edit_config(); either move this override out of edit_config() and apply it in the caller(s) (e.g., add the dasel command in create_validators and/or create_full_nodes where dev-only overrides are applied), or explicitly update the directive comment in edit_config() to document this exception; ensure references to cmd/config.go remain untouched and keep edit_config() focused on shared/default edits while placing environment-specific changes in the calling site(s).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@protocol/testing/testnet-dev/dev.sh`:
- Around line 232-234: The change sets '.consensus.timeout_commit' to '10s' in
dev.sh but it's unclear if this is temporary; either remove this from main
dev.sh and place it in the local-only script (e.g., local.sh) or gate it behind
an explicit flag (e.g., an env var like SLOW_COMMIT=true) so it doesn't affect
all users, and update the PR description/comments to document the intent and
rationale; locate the dasel command that writes '.consensus.timeout_commit' in
dev.sh and either move it to the local testnet script or wrap it with a guard
and add a brief comment explaining the chosen value.
---
Nitpick comments:
In `@protocol/testing/testnet-dev/dev.sh`:
- Around line 232-233: The new dasel change to set '.consensus.timeout_commit'
to '10s' violates the "DO NOT add more config modifications" rule in
edit_config(); either move this override out of edit_config() and apply it in
the caller(s) (e.g., add the dasel command in create_validators and/or
create_full_nodes where dev-only overrides are applied), or explicitly update
the directive comment in edit_config() to document this exception; ensure
references to cmd/config.go remain untouched and keep edit_config() focused on
shared/default edits while placing environment-specific changes in the calling
site(s).
| # slow down block time for easier troubleshooting. | ||
| dasel put -t string -f "$CONFIG_FOLDER"/config.toml '.consensus.timeout_commit' -v '10s' | ||
|
|
There was a problem hiding this comment.
Clarify whether this timeout_commit = 10s change is permanent or temporary.
The comment "for easier troubleshooting" and the commit message ("slow down block processign on dev") both read like a temporary debug aid rather than a permanent configuration. A 10s commit timeout significantly degrades dev testnet block throughput for all users of the environment. If this is only needed for local deferred-matching testing, it should not land in main — use the local testnet script (local.sh) exclusively, or guard it explicitly.
If it is intended to be permanent, the PR description should document the rationale and the chosen value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@protocol/testing/testnet-dev/dev.sh` around lines 232 - 234, The change sets
'.consensus.timeout_commit' to '10s' in dev.sh but it's unclear if this is
temporary; either remove this from main dev.sh and place it in the local-only
script (e.g., local.sh) or gate it behind an explicit flag (e.g., an env var
like SLOW_COMMIT=true) so it doesn't affect all users, and update the PR
description/comments to document the intent and rationale; locate the dasel
command that writes '.consensus.timeout_commit' in dev.sh and either move it to
the local testnet script or wrap it with a guard and add a brief comment
explaining the chosen value.
1 participant
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Tests
Chores