chore: configure automatic market settlement triggers #1307
Conversation
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
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
📝 WalkthroughWalkthroughAdds a ShutdownContext to tn_settlement and uses it for scheduler lifecycle; introduces an EngineOps interface and attestation request flow with ABI-decoding of stored query_components and fresh-read DB pool support; adds settlement_config migration/actions; updates ctor call sites and tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as "Scheduler\n(cron)"
participant EngineOps as "EngineOps\n(RequestAttestation)"
participant DB as "Database\n(query_components)"
participant Signer as "Signer\n(GetUserSigner)"
participant Broadcaster as "Broadcaster\n(tx broadcast)"
rect rgba(135,206,235,0.5)
Scheduler->>EngineOps: RunOnce -> FindUnsettledMarkets()
Scheduler->>EngineOps: For market without attestation -> RequestAttestationForMarket(ctx, market)
end
rect rgba(144,238,144,0.5)
EngineOps->>DB: GetMarketQueryComponents(queryID)
DB-->>EngineOps: returns ABI-encoded query_components ([]byte)
EngineOps->>EngineOps: decodeQueryComponents -> QueryComponents
EngineOps->>Signer: Sign request_attestation payload
Signer-->>EngineOps: signature
EngineOps->>Broadcaster: Broadcast signed tx
Broadcaster-->>EngineOps: tx result / hash
EngineOps-->>Scheduler: report success/failure
Scheduler->>Scheduler: continue processing markets
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@extensions/tn_settlement/scheduler/scheduler_test.go`:
- Around line 371-422: The test never asserts the job ran because jobExecuted is
never signaled; replace the nil EngineOps with a mock implementation of the
EngineOps interface (or a minimal struct) whose settleMarkets (or the method
invoked by the scheduler job) writes to the jobExecuted channel when called,
pass that mock into NewSettlementScheduler, then change the Sleep-based check to
wait for a value from jobExecuted with a timeout (e.g., select on <-jobExecuted
and time.After) before stopping the scheduler; update
TestSchedulerJobRunsAfterParentContextCanceled to reference the mock and the
settleMarkets method so the test fails if the job never executes.
In `@internal/migrations/041-settlement-config-actions.sql`:
- Line 1: The migration header inside the file 041-settlement-config-actions.sql
is incorrect (it reads "MIGRATION 042"); update the top comment to match the
filename (change "MIGRATION 042" to "MIGRATION 041") so the migration number in
the header and the file name are consistent; verify the comment string at the
top of the file that currently contains "MIGRATION 042" and replace it with
"MIGRATION 041".
🧹 Nitpick comments (1)
tests/streams/hoodi_bridge_coexistence_test.go (1)
39-46: Package-level mutable state may cause issues if tests run in parallel.The variables
ttPointCounter,tt2PointCounter,ttPrevPoint, andtt2PrevPointare package-level mutable state. While each test function resetsttPrevPointandtt2PrevPoint, the counters are only incremented, never reset. If these tests or other tests in the package run in parallel (t.Parallel()), this could lead to data races.Consider either:
- Ensuring these tests never run in parallel (current implicit behavior)
- Moving these variables into a test-local struct passed between helper functions
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@extensions/tn_settlement/scheduler/scheduler.go`:
- Around line 258-268: In RunOnce, don’t swallow errors from
engineOps.AttestationExists — replace the current "continue" on err with
handling that surfaces the failure: log the error with s.logger (include
"query_id" and the error) and return or propagate the error to the caller so the
caller can detect failure; adjust the block around
engineOps.AttestationExists(ctx, market.Hash) (and keep the existing
RequestAttestationForMarket call unchanged) so failures from AttestationExists
are not silently ignored.
🧹 Nitpick comments (2)
extensions/tn_settlement/internal/engine_ops.go (1)
354-380: Make “market not found” vs. “invalid query_components” distinguishable.
Right now, a row with NULL/invalidquery_componentsis surfaced as “market not found,” which obscures data issues. Consider returning an explicit error for NULL or unexpected types.♻️ Proposed refinement
func (e *EngineOperations) GetMarketQueryComponents(ctx context.Context, queryID int) (*QueryComponents, error) { var queryComponentsBytes []byte var found bool + var foundRow bool err := e.engine.ExecuteWithoutEngineCtx(ctx, e.db, `SELECT query_components FROM ob_queries WHERE id = $query_id`, map[string]any{"query_id": int64(queryID)}, func(row *common.Row) error { if len(row.Values) >= 1 { - if bytes, ok := row.Values[0].([]byte); ok { - queryComponentsBytes = bytes - found = true - } + foundRow = true + if row.Values[0] == nil { + return fmt.Errorf("query_components is NULL for query_id=%d", queryID) + } + bytes, ok := row.Values[0].([]byte) + if !ok { + return fmt.Errorf("unexpected query_components type: %T", row.Values[0]) + } + queryComponentsBytes = bytes + found = true } return nil }) if err != nil { return nil, fmt.Errorf("fetch query_components: %w", err) } - if !found { + if !foundRow { return nil, fmt.Errorf("market not found: query_id=%d", queryID) } + if !found { + return nil, fmt.Errorf("query_components missing or invalid for query_id=%d", queryID) + } return decodeQueryComponents(queryComponentsBytes) }extensions/tn_settlement/internal/engine_ops_test.go (1)
615-646: Guard against silent streamID truncation in test helper.
Copying into a[32]bytesilently truncates longer inputs. A small length check makes test failures clearer.♻️ Proposed tweak
func encodeQueryComponentsForTest(dataProvider, streamID, actionName string, argsBytes []byte) ([]byte, error) { + if len(streamID) > 32 { + return nil, errors.New("streamID must be <= 32 bytes") + } addressType, err := gethAbi.NewType("address", "", nil) if err != nil { return nil, err }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@extensions/tn_settlement/scheduler/scheduler_test.go`:
- Around line 95-105: The mock in mockEngineOps (method FindUnsettledMarkets)
currently ignores the incoming ctx and can allow tests to pass when the job's
parent context is canceled; update FindUnsettledMarkets to check ctx.Err() at
the start and, if non-nil, call the test failure function (e.g., t.Fatal or fail
helper) via the onFindUnsettledMarkets callback or by making the mock take a
*testing.T so the mock fails the test; ensure this check is present in
mockEngineOps.FindUnsettledMarkets (and the other mock instances referenced
around lines 412-456) so the test fails immediately when ctx is canceled rather
than returning an empty slice.
🧹 Nitpick comments (1)
extensions/tn_settlement/internal/engine_ops_test.go (1)
6-13: TightenArgsBytesassertions to verify content, not just length.
A wrong payload with the same length would pass today; a byte-wise check makes the test more precise.🔧 Suggested tweak
import ( + "bytes" "context" "errors" "fmt" @@ - if len(decoded.ArgsBytes) != len(argsBytes) { - t.Errorf("ArgsBytes length mismatch: expected %d, got %d", len(argsBytes), len(decoded.ArgsBytes)) - } + if !bytes.Equal(decoded.ArgsBytes, argsBytes) { + t.Errorf("ArgsBytes mismatch: expected %v, got %v", argsBytes, decoded.ArgsBytes) + }Also applies to: 477-509
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@extensions/tn_settlement/scheduler_lifecycle.go`:
- Around line 127-128: The scheduler is started with context.Background() in
startScheduler which prevents cancellation on graceful node shutdown and can
leave the scheduler running even if leaderwatch callbacks (settlementLeaderLose)
never fire; change the startup to derive the scheduler context from a node-level
shutdown context (or Extension's context) so it is cancelled on process
shutdown, or ensure Extension.Close() is invoked by the node shutdown sequence;
specifically, replace the Background() usage with a cancellable parent context
tied to the node/Extension lifecycle (use the existing Extension context or a
passed-in shutdownCtx) and keep the explicit scheduler.Stop() call in
Extension.Close() so the scheduler reliably stops on shutdown.
🧹 Nitpick comments (1)
extensions/tn_settlement/internal/engine_ops_test.go (1)
653-675: Consider adding a compile-time interface check.The mock implementation has stub methods that return
nil. Adding a compile-time assertion ensures the mock stays in sync if thecommon.Engineinterface changes.🔧 Suggested improvement
// Mock engine for query components test type mockEngineForQueryComponents struct { queryComponents []byte } + +// Compile-time check that mockEngineForQueryComponents implements common.Engine +var _ common.Engine = (*mockEngineForQueryComponents)(nil)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/migrations/037-order-book-validation.sql`:
- Around line 108-143: The aggregates $total_shares_all_markets and
$total_buys_cents_all_markets can overflow as BIGINT when summing across
markets; change the SELECT SUM(...) casts to NUMERIC (avoid ::BIGINT), declare
those variables as NUMERIC instead of BIGINT, and propagate NUMERIC types into
the downstream calculations (shares_collateral, buys_collateral,
expected_collateral) so all arithmetic is done in NUMERIC to prevent overflow
before converting or comparing as needed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
resolves: https://github.com/truflation/website/issues/3205
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.