add Settings domain with DB persistence and admin CRUD API#939
add Settings domain with DB persistence and admin CRUD API#939bitcoin-coder-bob wants to merge 30 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a persistent, runtime-configurable Settings domain with Get/Update/Clear admin APIs, DB migrations and repo implementations (badger/sqlite/postgres), wiring to load/apply settings at startup and runtime, plus docs, Makefile, env and e2e test updates to use admin-managed settings. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GRPC as "gRPC Handler"
participant AdminApp as "Admin Service"
participant RepoMgr as "Repo Manager"
participant SettingsRepo as "Settings Repo"
participant AppSvc as "App Service"
Client->>GRPC: UpdateSettings(request)
GRPC->>AdminApp: UpdateSettings(settings)
AdminApp->>AdminApp: validate & set UpdatedAt
AdminApp->>RepoMgr: Settings()
RepoMgr-->>AdminApp: SettingsRepository
AdminApp->>SettingsRepo: Upsert(settings)
SettingsRepo->>SettingsRepo: persist to storage
SettingsRepo-->>AdminApp: OK
AdminApp->>AppSvc: onSettingsUpdated / UpdateSettings(settings)
AppSvc->>AppSvc: recalc derived values & tapscript
AppSvc-->>AdminApp: OK
AdminApp-->>GRPC: UpdateSettingsResponse
GRPC-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
…-and-repo # Conflicts: # api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.go
# Conflicts: # internal/config/config.go
🔍 Arkana PR Review — arkd#939: Settings Domain with DB PersistenceScope: Moves 16 runtime-tunable config values (ban threshold/duration, exit delays, VTXO tree expiry, round participant limits, amount limits, etc.) from environment variables into a DB-persisted Settings domain with admin CRUD API. Touches 38 files, +2413/−199 lines. Overall: Well-structured domain model following the existing repository pattern. Clean separation across Postgres, SQLite, and Badger backends with proper migrations. Good test coverage. A few concerns below, some security-relevant. 🔴 Security: No Input Validation on UpdateSettings
Risks:
Suggestion: Add a Also consider using proto 🟡 Bug: ClearSettings Doesn't Reset Running Service
func (a *adminService) ClearSettings(ctx context.Context) error {
if err := a.repoManager.Settings().Clear(ctx); err != nil {
return err
}
// Missing: reset service to default settings
a.onInfoChange()
return nil
}Should either: (1) re-seed defaults and call 🟡 Duplicated Locktime Logic
Suggestion: Extract the shared logic into a single function in 🟢 Positives
Minor Notes
Verdict: The architectural approach is solid and the implementation is thorough. The missing input validation on UpdateSettings is the most critical item — it's a live admin API that directly controls protocol-security parameters (exit delays, VTXO expiry). Would recommend addressing the validation gap before merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
Makefile (2)
184-200: Remove staleMIN/MAXhandling fromrun-simulationoutput and shell vars.Line 197-Line 199 compute/log
MINandMAX, but they are never passed to the test command. Ininternal/test/e2e/single_batch_smoke_test.go(Line 23-Line 65), only-num-clientsis parsed, while min/max participants are hardcoded in test config. This log is misleading.Proposed cleanup
`@bash` -c '\ CLIENTS="$${CLIENTS:-5}"; \ - MIN="$${MIN:-$$CLIENTS}"; \ - MAX="$${MAX:-128}"; \ - echo "Running batch settlement test with $$CLIENTS clients (MIN=$$MIN, MAX=$$MAX)..."; \ + echo "Running batch settlement test with $$CLIENTS clients..."; \ go test -v -count=1 -timeout 1200s github.com/arkade-os/arkd/internal/test/e2e -run TestBatchSettleMultipleClients -args -smoke -num-clients=$$CLIENTS; \ '🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 184 - 200, The run-simulation Makefile target prints and sets unused MIN and MAX variables which is misleading; remove the MIN and MAX assignments and their use in the echo so the target only reads CLIENTS and passes -num-clients to the test. Update the run-simulation recipe's shell block to drop MIN="$${MIN:-$$CLIENTS}" and MAX="$${MAX:-128}" and change the echo to only reference $$CLIENTS (leaving the go test invocation that runs TestBatchSettleMultipleClients unchanged); this aligns the Makefile with the test in internal/test/e2e/single_batch_smoke_test.go which only parses -num-clients.
191-192: MakeARKD_SESSION_DURATIONoverrideable while keeping default60.Line 192 hardcodes
ARKD_SESSION_DURATION=60. Consider defaulting to 60 but allowing callers to override (make run-simulation ARKD_SESSION_DURATION=120) for troubleshooting slower environments.Proposed tweak
- `@ARKD_SESSION_DURATION`=60 docker compose -f docker-compose.regtest.yml up --build -d + `@ARKD_SESSION_DURATION`=$${ARKD_SESSION_DURATION:-60} docker compose -f docker-compose.regtest.yml up --build -d🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 191 - 192, The recipe currently hardcodes ARKD_SESSION_DURATION=60 in the Docker command; make this default-but-overridable by either (a) adding a Makefile variable declaration like ARKD_SESSION_DURATION ?= 60 and using ARKD_SESSION_DURATION in the recipe, or (b) using shell expansion in the recipe ARKD_SESSION_DURATION=${ARKD_SESSION_DURATION:-60} docker compose ... so callers can run make run-simulation ARKD_SESSION_DURATION=120 to override while keeping 60 as the default.internal/core/application/service.go (1)
304-349: Consider thread safety for runtime settings updates.
UpdateSettingsmodifies multiple service fields that may be read concurrently by round processing goroutines (e.g.,banDuration,roundMinParticipantsCount,checkpointExitDelay). Without synchronization, this could cause data races.Options to consider:
- Document that settings changes take effect between rounds (eventual consistency)
- Add a mutex to protect settings reads/writes
- Use atomic operations for individual fields
If the current behavior (unsynchronized updates) is intentional for simplicity, consider adding a comment documenting when it's safe to call this method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/service.go` around lines 304 - 349, The UpdateSettings method mutates many runtime fields (e.g., banDuration, banThreshold, unilateralExitDelay, publicUnilateralExitDelay, checkpointExitDelay, boardingExitDelay, batchExpiry, roundMinParticipantsCount, roundMaxParticipantsCount, vtxoMinAmount, vtxoMaxAmount, utxoMinAmount, utxoMaxAmount, settlementMinExpiryGap, vtxoNoCsvValidationCutoffTime, maxTxWeight, checkpointTapscript, forfeitPubkey) without synchronization; add a sync.RWMutex (e.g., settingsMu) to the service struct and wrap the entire UpdateSettings body with settingsMu.Lock()/Unlock(), and update all readers of those fields (round processing goroutines, any methods reading these fields) to use settingsMu.RLock()/RUnlock(); alternatively, if unsynchronized updates are intentional, add a clear comment on UpdateSettings documenting that it must only be called between rounds and is not concurrency-safe.internal/core/domain/settings.go (1)
25-55: Avoid the 16-argument positional constructor.All of these parameters are
int64, so a single swap still compiles and silently assigns the wrong setting. A dedicated input struct (or plain struct literal at call sites) is much safer to review and extend.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/domain/settings.go` around lines 25 - 55, The NewSettings function takes 16 positional int64 arguments which risks silent swaps; replace it with a single explicit config struct (e.g. SettingsConfig or SettingsOptions) containing named fields matching Settings (BanThreshold, BanDuration, UnilateralExitDelay, PublicUnilateralExitDelay, CheckpointExitDelay, BoardingExitDelay, VtxoTreeExpiry, RoundMinParticipantsCount, RoundMaxParticipantsCount, VtxoMinAmount, VtxoMaxAmount, UtxoMinAmount, UtxoMaxAmount, SettlementMinExpiryGap, VtxoNoCsvValidationCutoffDate, MaxTxWeight), change NewSettings signature to accept that struct (or a pointer) and construct &Settings from the struct fields, and update all callers to pass a struct literal (or use functional options) instead of positional args so each value is explicit and reviewable.api-spec/protobuf/ark/v1/admin.proto (1)
381-408: Keepupdated_atout of the update payload.
internal/interface/grpc/handlers/adminservice.goignoresreq.settings.updated_at, so reusing the sameSettingsmessage for reads and writes advertises a writable field the server never honors. A dedicated update message keeps the contract accurate and avoids clients round-tripping server-managed data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-spec/protobuf/ark/v1/admin.proto` around lines 381 - 408, The proto reuses message Settings for both reads and writes while server ignores req.settings.updated_at; change the write contract so clients cannot send updated_at by adding a dedicated update message (e.g., message SettingsUpdate or SettingsWritable) that mirrors Settings but omits the updated_at field, then change UpdateSettingsRequest to use that new message instead of Settings; update any generated stubs/clients and ensure adminservice.go handler continues to operate unchanged (and that server-managed updated_at remains set server-side).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api-spec/openapi/swagger/ark/v1/admin.openapi.json`:
- Around line 1695-1768: The UpdateSettingsRequest/Settings schema must be
updated at the proto/message level to match the handler contract: decide whether
the endpoint is a full-replace or a patch and implement that explicitly—if
full-replace, make UpdateSettingsRequest.settings required and make each
Settings field required (remove writable updatedAt or mark updatedAt as
output/readOnly); if patch, change Settings fields to use nullable wrapper types
(e.g., google.protobuf.Int64Value) or a FieldMask so omitted fields are
distinguishable from zero and won’t overwrite stored values; update the Settings
message to not expose updatedAt as an input field (mark it output-only) and then
regen the OpenAPI/JSON so UpdateSettingsRequest aligns with adminservice's
behavior (refer to UpdateSettingsRequest, Settings, and the adminservice handler
logic).
In `@internal/core/application/admin.go`:
- Around line 634-639: ClearSettings removes persisted settings but doesn't
update the in-memory runtime config; call the settings update handler after
clearing so the node re-seeds defaults and applies the change. Modify
ClearSettings (the function that calls a.repoManager.Settings().Clear) to invoke
a.onSettingsUpdated() (in addition to or instead of a.onInfoChange())
immediately after the clear succeeds so the in-memory settings and any
subscribers are refreshed (Config.loadSettings is the startup equivalent to
emulate).
- Around line 620-630: The UpdateSettings flow currently persists settings
(repoManager.Settings().Upsert) before running live-apply guards, which can lead
to races and invalid persisted values; change UpdateSettings (method
adminService.UpdateSettings) to serialize concurrent updates (add and use a
mutex field on adminService e.g., updateSettingsMu) and perform
validation/live-apply before making durable: acquire the mutex, run the same
validation/startup checks from internal/config/config.go and call
a.onSettingsUpdated(ctx, settings) while still in-memory, only if
validation/onSettingsUpdated succeeds then set settings.UpdatedAt and call
repoManager.Settings().Upsert(ctx, settings), finally release the mutex and call
a.onInfoChange(); propagate errors from validation/apply/upsert unchanged.
- Around line 620-631: UpdateSettings currently writes settings but does not
refresh the cached participant defaults
(roundMinParticipantsCount/roundMaxParticipantsCount), so subsequent
UpdateScheduledSessionConfig calls may still use startup defaults; after
successfully upserting settings and invoking a.onSettingsUpdated, update the
adminService cached fields by assigning a.roundMinParticipantsCount and
a.roundMaxParticipantsCount from the new settings (or call an existing helper
that derives those defaults), ensuring this happens before a.onInfoChange() so
future calls to UpdateScheduledSessionConfig use the new values.
In `@internal/core/application/service.go`:
- Around line 295-302: Add a clarifying doc comment above the toRelativeLocktime
function explaining why values >= minAllowedSequence (512) are treated as
seconds and values < 512 as blocks; mention the BIP68 granularity (512-second
units) and that configuration validation enforces time-based values be multiples
of 512 so this threshold determines whether arklib.RelativeLocktime uses
arklib.LocktimeTypeSecond vs arklib.LocktimeTypeBlock.
In `@internal/infrastructure/db/badger/settings_repo.go`:
- Around line 64-79: The Upsert method in settingsRepository currently retries
on badger.ErrConflict without honoring cancellation; modify Upsert to stop
retrying as soon as ctx is canceled by checking ctx.Done() between attempts and
before sleeping/attempting another r.store.Upsert, returning ctx.Err() if
canceled; preserve existing retry count logic (maxRetries) and continue only
while ctx is active, referencing the Upsert method, settingsKey, maxRetries and
the same badger.ErrConflict check.
In `@internal/infrastructure/db/service.go`:
- Around line 229-232: The settings-store initialization can fail after the DB
connection and earlier repositories are created, leaving those handles open;
wrap resource creation in a cleanup stack or use a defer-until-success pattern:
after creating the DB connection and each repository, push their corresponding
cleanup functions (e.g., db.Close()/Disconnect and repo.Close()/Shutdown) onto a
slice of funcs, and if settingsStoreFactory(...) returns an error, iterate that
slice to call each cleanup before returning the error; apply the same pattern
around the other failing sites noted (the analogous blocks at the later
occurrences referenced) so all partially-initialized resources are closed on
error.
In `@internal/interface/grpc/handlers/adminservice.go`:
- Around line 589-621: The UpdateSettings handler (adminHandler.UpdateSettings)
must validate invariants before calling a.adminService.UpdateSettings: ensure
numeric/duration fields are within acceptable bounds (non-negative for
BanThreshold, BanDuration, UnilateralExitDelay, PublicUnilateralExitDelay,
CheckpointExitDelay, BoardingExitDelay, VtxoTreeExpiry, SettlementMinExpiryGap,
MaxTxWeight), ensure participant counts satisfy RoundMinParticipantsCount <
RoundMaxParticipantsCount, ensure min/max pairs are ordered (VtxoMinAmount <=
VtxoMaxAmount, UtxoMinAmount <= UtxoMaxAmount), and validate any date fields
(VtxoNoCsvValidationCutoffDate) as needed; if any check fails return
status.Error(codes.InvalidArgument, "...") with a clear message. Implement these
checks at the start of adminHandler.UpdateSettings (before calling
a.adminService.UpdateSettings / Upsert) and use the same status codes/messages
pattern as other handlers.
In `@README.md`:
- Around line 102-122: The README admin-settings section is out of sync: update
the documentation to list and describe the newly added endpoints GET
/v1/admin/settings and POST /v1/admin/settings/clear in addition to POST
/v1/admin/settings, and extend the settings table to include the two missing
persisted settings settlement_min_expiry_gap and
vtxo_no_csv_validation_cutoff_date with their descriptions and default values;
ensure the table rows use the same formatting as existing entries (backticked
keys like `settlement_min_expiry_gap`, `vtxo_no_csv_validation_cutoff_date`) and
mention which endpoints manage them.
---
Nitpick comments:
In `@api-spec/protobuf/ark/v1/admin.proto`:
- Around line 381-408: The proto reuses message Settings for both reads and
writes while server ignores req.settings.updated_at; change the write contract
so clients cannot send updated_at by adding a dedicated update message (e.g.,
message SettingsUpdate or SettingsWritable) that mirrors Settings but omits the
updated_at field, then change UpdateSettingsRequest to use that new message
instead of Settings; update any generated stubs/clients and ensure
adminservice.go handler continues to operate unchanged (and that server-managed
updated_at remains set server-side).
In `@internal/core/application/service.go`:
- Around line 304-349: The UpdateSettings method mutates many runtime fields
(e.g., banDuration, banThreshold, unilateralExitDelay,
publicUnilateralExitDelay, checkpointExitDelay, boardingExitDelay, batchExpiry,
roundMinParticipantsCount, roundMaxParticipantsCount, vtxoMinAmount,
vtxoMaxAmount, utxoMinAmount, utxoMaxAmount, settlementMinExpiryGap,
vtxoNoCsvValidationCutoffTime, maxTxWeight, checkpointTapscript, forfeitPubkey)
without synchronization; add a sync.RWMutex (e.g., settingsMu) to the service
struct and wrap the entire UpdateSettings body with settingsMu.Lock()/Unlock(),
and update all readers of those fields (round processing goroutines, any methods
reading these fields) to use settingsMu.RLock()/RUnlock(); alternatively, if
unsynchronized updates are intentional, add a clear comment on UpdateSettings
documenting that it must only be called between rounds and is not
concurrency-safe.
In `@internal/core/domain/settings.go`:
- Around line 25-55: The NewSettings function takes 16 positional int64
arguments which risks silent swaps; replace it with a single explicit config
struct (e.g. SettingsConfig or SettingsOptions) containing named fields matching
Settings (BanThreshold, BanDuration, UnilateralExitDelay,
PublicUnilateralExitDelay, CheckpointExitDelay, BoardingExitDelay,
VtxoTreeExpiry, RoundMinParticipantsCount, RoundMaxParticipantsCount,
VtxoMinAmount, VtxoMaxAmount, UtxoMinAmount, UtxoMaxAmount,
SettlementMinExpiryGap, VtxoNoCsvValidationCutoffDate, MaxTxWeight), change
NewSettings signature to accept that struct (or a pointer) and construct
&Settings from the struct fields, and update all callers to pass a struct
literal (or use functional options) instead of positional args so each value is
explicit and reviewable.
In `@Makefile`:
- Around line 184-200: The run-simulation Makefile target prints and sets unused
MIN and MAX variables which is misleading; remove the MIN and MAX assignments
and their use in the echo so the target only reads CLIENTS and passes
-num-clients to the test. Update the run-simulation recipe's shell block to drop
MIN="$${MIN:-$$CLIENTS}" and MAX="$${MAX:-128}" and change the echo to only
reference $$CLIENTS (leaving the go test invocation that runs
TestBatchSettleMultipleClients unchanged); this aligns the Makefile with the
test in internal/test/e2e/single_batch_smoke_test.go which only parses
-num-clients.
- Around line 191-192: The recipe currently hardcodes ARKD_SESSION_DURATION=60
in the Docker command; make this default-but-overridable by either (a) adding a
Makefile variable declaration like ARKD_SESSION_DURATION ?= 60 and using
ARKD_SESSION_DURATION in the recipe, or (b) using shell expansion in the recipe
ARKD_SESSION_DURATION=${ARKD_SESSION_DURATION:-60} docker compose ... so callers
can run make run-simulation ARKD_SESSION_DURATION=120 to override while keeping
60 as the default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ef746c5-f1c4-43ba-8188-e57d581bb56a
⛔ Files ignored due to path filters (4)
api-spec/protobuf/gen/ark/v1/admin.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/admin.pb.rgw.gois excluded by!**/gen/**api-spec/protobuf/gen/ark/v1/admin_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.gois excluded by!**/gen/**
📒 Files selected for processing (34)
MakefileREADME.mdapi-spec/openapi/swagger/ark/v1/admin.openapi.jsonapi-spec/protobuf/ark/v1/admin.protodocker-compose.regtest.ymlenvs/arkd.dev.envenvs/arkd.light.envinternal/config/config.gointernal/core/application/admin.gointernal/core/application/admin_test.gointernal/core/application/service.gointernal/core/application/types.gointernal/core/domain/settings.gointernal/core/domain/settings_repo.gointernal/core/ports/repo_manager.gointernal/infrastructure/db/badger/settings_repo.gointernal/infrastructure/db/postgres/migration/20260227000000_add_settings.down.sqlinternal/infrastructure/db/postgres/migration/20260227000000_add_settings.up.sqlinternal/infrastructure/db/postgres/settings_repo.gointernal/infrastructure/db/postgres/sqlc/queries/models.gointernal/infrastructure/db/postgres/sqlc/queries/query.sql.gointernal/infrastructure/db/postgres/sqlc/query.sqlinternal/infrastructure/db/service.gointernal/infrastructure/db/service_test.gointernal/infrastructure/db/sqlite/migration/20260227000000_add_settings.down.sqlinternal/infrastructure/db/sqlite/migration/20260227000000_add_settings.up.sqlinternal/infrastructure/db/sqlite/settings_repo.gointernal/infrastructure/db/sqlite/sqlc/queries/models.gointernal/infrastructure/db/sqlite/sqlc/queries/query.sql.gointernal/infrastructure/db/sqlite/sqlc/query.sqlinternal/interface/grpc/handlers/adminservice.gointernal/interface/grpc/permissions/permissions.gointernal/test/e2e/single_batch_smoke_test.gointernal/test/e2e/utils_test.go
💤 Files with no reviewable changes (2)
- envs/arkd.light.env
- envs/arkd.dev.env
| "Settings": { | ||
| "title": "Settings", | ||
| "type": "object", | ||
| "properties": { | ||
| "banDuration": { | ||
| "type": "integer", | ||
| "format": "int64" | ||
| }, | ||
| "banThreshold": { | ||
| "type": "integer", | ||
| "format": "int64" | ||
| }, | ||
| "boardingExitDelay": { | ||
| "type": "integer", | ||
| "format": "int64" | ||
| }, | ||
| "checkpointExitDelay": { | ||
| "type": "integer", | ||
| "format": "int64" | ||
| }, | ||
| "maxTxWeight": { | ||
| "type": "integer", | ||
| "format": "int64" | ||
| }, | ||
| "publicUnilateralExitDelay": { | ||
| "type": "integer", | ||
| "format": "int64" | ||
| }, | ||
| "roundMaxParticipantsCount": { | ||
| "type": "integer", | ||
| "format": "int64" | ||
| }, | ||
| "roundMinParticipantsCount": { | ||
| "type": "integer", | ||
| "format": "int64" | ||
| }, | ||
| "settlementMinExpiryGap": { | ||
| "type": "integer", | ||
| "format": "int64" | ||
| }, | ||
| "unilateralExitDelay": { | ||
| "type": "integer", | ||
| "format": "int64" | ||
| }, | ||
| "updatedAt": { | ||
| "type": "integer", | ||
| "format": "int64" | ||
| }, | ||
| "utxoMaxAmount": { | ||
| "type": "integer", | ||
| "format": "int64" | ||
| }, | ||
| "utxoMinAmount": { | ||
| "type": "integer", | ||
| "format": "int64" | ||
| }, | ||
| "vtxoMaxAmount": { | ||
| "type": "integer", | ||
| "format": "int64" | ||
| }, | ||
| "vtxoMinAmount": { | ||
| "type": "integer", | ||
| "format": "int64" | ||
| }, | ||
| "vtxoNoCsvValidationCutoffDate": { | ||
| "type": "integer", | ||
| "format": "int64" | ||
| }, | ||
| "vtxoTreeExpiry": { | ||
| "type": "integer", | ||
| "format": "int64" | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
UpdateSettingsRequest does not match the handler’s real contract.
The schema makes settings optional, makes every nested field optional, and reuses the response model with writable-looking updatedAt. In internal/interface/grpc/handlers/adminservice.go, Lines 592-614, {} is rejected, omitted fields are read as zero and overwrite stored settings, and updatedAt is ignored. Please fix this at the proto/message level before regen so the published contract is either an explicit full-replace shape or a true patch shape.
Based on learnings, the project prioritizes backward compatibility for API endpoints.
Also applies to: 1861-1869
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api-spec/openapi/swagger/ark/v1/admin.openapi.json` around lines 1695 - 1768,
The UpdateSettingsRequest/Settings schema must be updated at the proto/message
level to match the handler contract: decide whether the endpoint is a
full-replace or a patch and implement that explicitly—if full-replace, make
UpdateSettingsRequest.settings required and make each Settings field required
(remove writable updatedAt or mark updatedAt as output/readOnly); if patch,
change Settings fields to use nullable wrapper types (e.g.,
google.protobuf.Int64Value) or a FieldMask so omitted fields are distinguishable
from zero and won’t overwrite stored values; update the Settings message to not
expose updatedAt as an input field (mark it output-only) and then regen the
OpenAPI/JSON so UpdateSettingsRequest aligns with adminservice's behavior (refer
to UpdateSettingsRequest, Settings, and the adminservice handler logic).
|
addressed the comments from @arkanaai (#939 (comment)) in: 5271785 |
|
Thanks for addressing the feedback, the changes look good overall. A few notes on the commit: ✅ Locktime deduplication — clean, / is the right home. ✅ ClearSettings — now correctly re-seeds defaults and calls ✅ Validate() — covers the critical cases (zero exit delays, min/max participants ordering, public ≥ unilateral). Two small follow-up items:
Neither is a blocker, but worth a quick fix before merge. |
🔍 PR Review: Settings Domain with DB Persistence and Admin CRUD APIReviewer: Arkana (automated) SummaryThis PR moves ~15 operator-configurable parameters (exit delays, VTXO tree expiry, round limits, ban thresholds, amount bounds, etc.) out of environment variables into a new ✅ What looks good
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/core/application/admin.go (2)
631-637:⚠️ Potential issue | 🟡 MinorCached participant count fields not refreshed after settings update.
After
UpdateSettingssucceeds,a.roundMinParticipantsCountanda.roundMaxParticipantsCountstill hold their startup values. This affectsUpdateScheduledSessionConfigwhich uses these as fallbacks (lines 279-283, 304-308).Suggested fix
if a.onSettingsUpdated != nil { if err := a.onSettingsUpdated(ctx, settings); err != nil { return err } } + a.roundMinParticipantsCount = settings.RoundMinParticipantsCount + a.roundMaxParticipantsCount = settings.RoundMaxParticipantsCount a.onInfoChange() return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/admin.go` around lines 631 - 637, After UpdateSettings succeeds the cached fields a.roundMinParticipantsCount and a.roundMaxParticipantsCount are not being refreshed, so UpdateScheduledSessionConfig still sees old startup values; after the successful call to a.onSettingsUpdated (i.e. inside the same block where you handle the non-nil callback) update the cached values from the new settings (assign settings.RoundMinParticipantsCount and settings.RoundMaxParticipantsCount into a.roundMinParticipantsCount and a.roundMaxParticipantsCount) before calling a.onInfoChange so subsequent UpdateScheduledSessionConfig calls use the updated fallbacks.
623-638:⚠️ Potential issue | 🟠 MajorPersist-then-apply ordering can leave inconsistent state on callback failure.
If
onSettingsUpdatedfails afterUpsertsucceeds, the database contains the new settings while the runtime keeps the old configuration. Without transaction support (per theSettingsRepositoryinterface), consider reversing the order: apply first, persist on success.Suggested reordering
func (a *adminService) UpdateSettings(ctx context.Context, settings domain.Settings) error { if err := settings.Validate(); err != nil { return fmt.Errorf("invalid settings: %w", err) } - settings.UpdatedAt = time.Now() - if err := a.repoManager.Settings().Upsert(ctx, settings); err != nil { - return err - } if a.onSettingsUpdated != nil { if err := a.onSettingsUpdated(ctx, settings); err != nil { return err } } + settings.UpdatedAt = time.Now() + if err := a.repoManager.Settings().Upsert(ctx, settings); err != nil { + return err + } a.onInfoChange() return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/admin.go` around lines 623 - 638, UpdateSettings currently persists settings via Settings().Upsert before calling the runtime callback onSettingsUpdated, risking DB/runtime divergence if the callback fails; change the order so the in-memory/runtime application happens first (call a.onSettingsUpdated(ctx, settings) if non-nil), only if that succeeds then call a.repoManager.Settings().Upsert(ctx, settings), and finally call a.onInfoChange(); ensure you still validate and set settings.UpdatedAt before applying, and propagate errors from the callback or the Upsert unchanged so failures abort without leaving the runtime/stored state inconsistent.
🧹 Nitpick comments (2)
internal/core/domain/settings.go (1)
39-69: Consider adding validation forCheckpointExitDelayandMaxTxWeight.The
Validate()method checksUnilateralExitDelay,BoardingExitDelay, andVtxoTreeExpiryfor positive values, but omitsCheckpointExitDelayandMaxTxWeight. For consistency and to prevent invalid configurations:Suggested additions
func (s Settings) Validate() error { if s.UnilateralExitDelay <= 0 { return fmt.Errorf("unilateral exit delay must be greater than 0") } + if s.CheckpointExitDelay <= 0 { + return fmt.Errorf("checkpoint exit delay must be greater than 0") + } if s.BoardingExitDelay <= 0 { return fmt.Errorf("boarding exit delay must be greater than 0") } // ... existing checks ... + if s.MaxTxWeight <= 0 { + return fmt.Errorf("max tx weight must be greater than 0") + } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/domain/settings.go` around lines 39 - 69, Add validation in Settings.Validate to check CheckpointExitDelay and MaxTxWeight are positive, mirroring existing checks: after the existing UnilateralExitDelay/BoardingExitDelay/VtxoTreeExpiry checks add an if s.CheckpointExitDelay <= 0 { return fmt.Errorf("checkpoint exit delay must be greater than 0") } and an if s.MaxTxWeight <= 0 { return fmt.Errorf("max tx weight must be greater than 0") }, keeping the checks inside the Settings.Validate function.internal/config/config.go (1)
369-371: Consider inliningdetermineLocktimeType.This function is now a simple wrapper around
domain.ToRelativeLocktime. Consider removing the indirection:-func determineLocktimeType(locktime int64) arklib.RelativeLocktime { - return domain.ToRelativeLocktime(locktime) -}Then call
domain.ToRelativeLocktime()directly inloadSettings().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 369 - 371, The helper function determineLocktimeType is an unnecessary wrapper around domain.ToRelativeLocktime; remove the determineLocktimeType function and update callers (e.g., loadSettings) to call domain.ToRelativeLocktime(locktime) directly, ensuring any imports or references remain correct and tests/usage of loadSettings continue to compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/application/admin.go`:
- Around line 640-656: ClearSettings currently does Clear + Upsert before
calling onSettingsUpdated and does not refresh cached participant counts; change
the ordering to match UpdateSettings by: perform
repoManager.Settings().Clear(ctx), set defaults.UpdatedAt, call
a.onSettingsUpdated(ctx, defaults) (if non-nil) before persisting, then Upsert
the defaults, and after successful Upsert refresh the cached participant counts
using the same helper used by UpdateSettings and finally call a.onInfoChange();
ensure you propagate any errors from onSettingsUpdated, the refresh helper, and
Upsert.
In `@internal/core/application/service.go`:
- Around line 295-340: UpdateSettings currently mutates shared service fields
(banDuration, banThreshold, unilateralExitDelay, publicUnilateralExitDelay,
checkpointExitDelay, boardingExitDelay, batchExpiry, roundMinParticipantsCount,
roundMaxParticipantsCount, vtxoMinAmount, vtxoMaxAmount, utxoMinAmount,
utxoMaxAmount, settlementMinExpiryGap, vtxoNoCsvValidationCutoffTime,
maxTxWeight, checkpointTapscript) without synchronization; add a settings mutex
(e.g., s.settingsMu sync.RWMutex) and wrap the entire UpdateSettings body with
s.settingsMu.Lock()/Unlock(), then change readers that access these fields
(examples: startRound, startConfirmation) to use s.settingsMu.RLock()/RUnlock()
when reading to prevent data races and ensure consistent view of settings.
---
Duplicate comments:
In `@internal/core/application/admin.go`:
- Around line 631-637: After UpdateSettings succeeds the cached fields
a.roundMinParticipantsCount and a.roundMaxParticipantsCount are not being
refreshed, so UpdateScheduledSessionConfig still sees old startup values; after
the successful call to a.onSettingsUpdated (i.e. inside the same block where you
handle the non-nil callback) update the cached values from the new settings
(assign settings.RoundMinParticipantsCount and
settings.RoundMaxParticipantsCount into a.roundMinParticipantsCount and
a.roundMaxParticipantsCount) before calling a.onInfoChange so subsequent
UpdateScheduledSessionConfig calls use the updated fallbacks.
- Around line 623-638: UpdateSettings currently persists settings via
Settings().Upsert before calling the runtime callback onSettingsUpdated, risking
DB/runtime divergence if the callback fails; change the order so the
in-memory/runtime application happens first (call a.onSettingsUpdated(ctx,
settings) if non-nil), only if that succeeds then call
a.repoManager.Settings().Upsert(ctx, settings), and finally call
a.onInfoChange(); ensure you still validate and set settings.UpdatedAt before
applying, and propagate errors from the callback or the Upsert unchanged so
failures abort without leaving the runtime/stored state inconsistent.
---
Nitpick comments:
In `@internal/config/config.go`:
- Around line 369-371: The helper function determineLocktimeType is an
unnecessary wrapper around domain.ToRelativeLocktime; remove the
determineLocktimeType function and update callers (e.g., loadSettings) to call
domain.ToRelativeLocktime(locktime) directly, ensuring any imports or references
remain correct and tests/usage of loadSettings continue to compile.
In `@internal/core/domain/settings.go`:
- Around line 39-69: Add validation in Settings.Validate to check
CheckpointExitDelay and MaxTxWeight are positive, mirroring existing checks:
after the existing UnilateralExitDelay/BoardingExitDelay/VtxoTreeExpiry checks
add an if s.CheckpointExitDelay <= 0 { return fmt.Errorf("checkpoint exit delay
must be greater than 0") } and an if s.MaxTxWeight <= 0 { return fmt.Errorf("max
tx weight must be greater than 0") }, keeping the checks inside the
Settings.Validate function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1b59888-eb73-40cb-9875-bd5d2eefd762
📒 Files selected for processing (6)
internal/config/config.gointernal/core/application/admin.gointernal/core/application/admin_test.gointernal/core/application/service.gointernal/core/domain/settings.gointernal/interface/grpc/handlers/adminservice.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/interface/grpc/handlers/adminservice.go
- internal/core/application/admin_test.go
| func (s *service) UpdateSettings(settings domain.Settings) error { | ||
| s.banDuration = time.Duration(settings.BanDuration) * time.Second | ||
| s.banThreshold = settings.BanThreshold | ||
| s.unilateralExitDelay = domain.ToRelativeLocktime(settings.UnilateralExitDelay) | ||
| s.publicUnilateralExitDelay = domain.ToRelativeLocktime(settings.PublicUnilateralExitDelay) | ||
| s.checkpointExitDelay = domain.ToRelativeLocktime(settings.CheckpointExitDelay) | ||
| s.boardingExitDelay = domain.ToRelativeLocktime(settings.BoardingExitDelay) | ||
| s.batchExpiry = domain.ToRelativeLocktime(settings.VtxoTreeExpiry) | ||
| s.roundMinParticipantsCount = settings.RoundMinParticipantsCount | ||
| s.roundMaxParticipantsCount = settings.RoundMaxParticipantsCount | ||
| s.vtxoMinAmount = settings.VtxoMinAmount | ||
| s.vtxoMaxAmount = settings.VtxoMaxAmount | ||
| s.utxoMinAmount = settings.UtxoMinAmount | ||
| s.utxoMaxAmount = settings.UtxoMaxAmount | ||
| s.settlementMinExpiryGap = time.Duration(settings.SettlementMinExpiryGap) * time.Second | ||
| s.vtxoNoCsvValidationCutoffTime = time.Unix(settings.VtxoNoCsvValidationCutoffDate, 0) | ||
| s.maxTxWeight = uint64(settings.MaxTxWeight) | ||
|
|
||
| // Recalculate dust-derived values. | ||
| ctx := context.Background() | ||
| dustAmount, err := s.wallet.GetDustAmount(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get dust amount: %s", err) | ||
| } | ||
|
|
||
| s.vtxoMinAmount, s.utxoMinAmount = resolveMinAmounts( | ||
| s.vtxoMinAmount, s.utxoMinAmount, int64(dustAmount), | ||
| ) | ||
|
|
||
| // Recalculate checkpoint tapscript if forfeit pubkey is already set. | ||
| if s.forfeitPubkey != nil { | ||
| checkpointClosure := &script.CSVMultisigClosure{ | ||
| Locktime: s.checkpointExitDelay, | ||
| MultisigClosure: script.MultisigClosure{ | ||
| PubKeys: []*btcec.PublicKey{s.forfeitPubkey}, | ||
| }, | ||
| } | ||
| checkpointTapscript, err := checkpointClosure.Script() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to encode checkpoint tapscript: %s", err) | ||
| } | ||
| s.checkpointTapscript = checkpointTapscript | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Data race: concurrent field updates without synchronization.
UpdateSettings modifies multiple shared service fields (banDuration, vtxoMinAmount, unilateralExitDelay, etc.) that are read concurrently by round execution goroutines (startRound, startConfirmation, etc.). Without mutex protection, this can cause data races and inconsistent state.
Consider adding a dedicated mutex for settings or reusing an existing one:
Suggested approach
type service struct {
+ settingsMu sync.RWMutex
// ... existing fields
}
func (s *service) UpdateSettings(settings domain.Settings) error {
+ s.settingsMu.Lock()
+ defer s.settingsMu.Unlock()
+
s.banDuration = time.Duration(settings.BanDuration) * time.Second
// ... rest of updates
}Then use s.settingsMu.RLock() in readers like startConfirmation when accessing these fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/application/service.go` around lines 295 - 340, UpdateSettings
currently mutates shared service fields (banDuration, banThreshold,
unilateralExitDelay, publicUnilateralExitDelay, checkpointExitDelay,
boardingExitDelay, batchExpiry, roundMinParticipantsCount,
roundMaxParticipantsCount, vtxoMinAmount, vtxoMaxAmount, utxoMinAmount,
utxoMaxAmount, settlementMinExpiryGap, vtxoNoCsvValidationCutoffTime,
maxTxWeight, checkpointTapscript) without synchronization; add a settings mutex
(e.g., s.settingsMu sync.RWMutex) and wrap the entire UpdateSettings body with
s.settingsMu.Lock()/Unlock(), then change readers that access these fields
(examples: startRound, startConfirmation) to use s.settingsMu.RLock()/RUnlock()
when reading to prevent data races and ensure consistent view of settings.
…x/min participants on ClearSettings, commenting, MaxTxWeight validation, badger settings repo ctx cancellation in retry loop in Upsert
🔍 Follow-up Review — New Commits (
|
|
addressed @arkanaai comment for string based error detction (#939 (comment)) in: 2802d62 however for VtxoTreeExpiry > UnilateralExitDelay while we can normalize the comparison using RelativeLocktime.Seconds(), the block scheduler defaults would fail it (vtxoTreeExpiry=20 blocks ≈ 12,000s vs this is the case that would break things, since now |
|
Makes sense — the block scheduler case is a legitimate exception and you're right that Skipping the Otherwise this looks good to me — both original blockers are resolved. |
🔍 Arkana Review — Follow-up (commit
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/core/application/admin.go (1)
652-673: Note:ClearSettingsapplies to the running service after persisting, unlikeUpdateSettings.In
UpdateSettings,onSettingsUpdatedis called beforeUpsertto fail-fast on live-apply errors. InClearSettings, it's called afterUpsert. IfonSettingsUpdatedwere to fail (unlikely given context snippet 1 shows it just assigns fields), the DB would have the new defaults while the runtime retains old values.Since
service.UpdateSettings(the concrete callback) only performs field assignments and cannot fail in practice, this ordering difference is low-risk. However, for consistency and defensive coding, consider aligning the ordering withUpdateSettings.💡 Optional: Align ordering with UpdateSettings
func (a *adminService) ClearSettings(ctx context.Context) error { a.settingsMu.Lock() defer a.settingsMu.Unlock() if err := a.repoManager.Settings().Clear(ctx); err != nil { return err } defaults := a.defaultSettings defaults.UpdatedAt = time.Now() + if a.onSettingsUpdated != nil { + if err := a.onSettingsUpdated(ctx, defaults); err != nil { + return err + } + } if err := a.repoManager.Settings().Upsert(ctx, defaults); err != nil { return err } - if a.onSettingsUpdated != nil { - if err := a.onSettingsUpdated(ctx, defaults); err != nil { - return err - } - } a.roundMinParticipantsCount = defaults.RoundMinParticipantsCount🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/admin.go` around lines 652 - 673, The ClearSettings workflow currently calls repoManager.Settings().Upsert(...) before invoking a.onSettingsUpdated(...), risking the DB being updated while the runtime fails to apply live changes; change ClearSettings to mirror UpdateSettings by calling a.onSettingsUpdated(ctx, defaults) before attempting a.repoManager.Settings().Upsert(ctx, defaults) so failures in onSettingsUpdated cause an early return and avoid persisting inconsistent state (keep locking behavior with a.settingsMu and leave subsequent assignments to a.roundMinParticipantsCount/a.roundMaxParticipantsCount and a.onInfoChange() as-is).internal/core/domain/settings.go (1)
53-88: Consider adding validation for amount min/max ordering when both are set.The
Validate()method checks several constraints but doesn't validate thatVtxoMinAmount <= VtxoMaxAmountandUtxoMinAmount <= UtxoMaxAmountwhen both are explicitly set (i.e., not-1). From the context,-1appears to mean "unset/unlimited", so validation should only apply when both values are positive.💡 Optional enhancement
if s.MaxTxWeight <= 0 { return &ErrInvalidSettings{"max tx weight must be greater than 0"} } + if s.VtxoMinAmount > 0 && s.VtxoMaxAmount > 0 && s.VtxoMinAmount > s.VtxoMaxAmount { + return &ErrInvalidSettings{"vtxo min amount must be <= vtxo max amount"} + } + if s.UtxoMinAmount > 0 && s.UtxoMaxAmount > 0 && s.UtxoMinAmount > s.UtxoMaxAmount { + return &ErrInvalidSettings{"utxo min amount must be <= utxo max amount"} + } return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/domain/settings.go` around lines 53 - 88, The Validate() method on Settings lacks checks ensuring min ≤ max for amount pairs; add validation in Settings.Validate to verify when both VtxoMinAmount and VtxoMaxAmount are set (i.e., != -1) that VtxoMinAmount <= VtxoMaxAmount and similarly when both UtxoMinAmount and UtxoMaxAmount are set that UtxoMinAmount <= UtxoMaxAmount, returning &ErrInvalidSettings with clear messages like "vtxo min amount must be <= vtxo max amount" and "utxo min amount must be <= utxo max amount" when the checks fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/domain/settings.go`:
- Around line 26-31: Add explicit upper-bound checks in Settings.Validate to
ensure delay fields do not overflow uint32: validate that UnilateralExitDelay,
BoardingExitDelay, and VtxoTreeExpiry are each <= math.MaxUint32 and return a
descriptive ErrInvalidSettings (e.g., "unilateral exit delay exceeds maximum
uint32 value") on violation; this prevents the silent truncation when
ToRelativeLocktime casts an int64 to uint32.
---
Nitpick comments:
In `@internal/core/application/admin.go`:
- Around line 652-673: The ClearSettings workflow currently calls
repoManager.Settings().Upsert(...) before invoking a.onSettingsUpdated(...),
risking the DB being updated while the runtime fails to apply live changes;
change ClearSettings to mirror UpdateSettings by calling
a.onSettingsUpdated(ctx, defaults) before attempting
a.repoManager.Settings().Upsert(ctx, defaults) so failures in onSettingsUpdated
cause an early return and avoid persisting inconsistent state (keep locking
behavior with a.settingsMu and leave subsequent assignments to
a.roundMinParticipantsCount/a.roundMaxParticipantsCount and a.onInfoChange()
as-is).
In `@internal/core/domain/settings.go`:
- Around line 53-88: The Validate() method on Settings lacks checks ensuring min
≤ max for amount pairs; add validation in Settings.Validate to verify when both
VtxoMinAmount and VtxoMaxAmount are set (i.e., != -1) that VtxoMinAmount <=
VtxoMaxAmount and similarly when both UtxoMinAmount and UtxoMaxAmount are set
that UtxoMinAmount <= UtxoMaxAmount, returning &ErrInvalidSettings with clear
messages like "vtxo min amount must be <= vtxo max amount" and "utxo min amount
must be <= utxo max amount" when the checks fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2021358-40fc-4c29-9846-8fa56c78afc1
📒 Files selected for processing (3)
internal/core/application/admin.gointernal/core/domain/settings.gointernal/interface/grpc/handlers/adminservice.go
|
for the commnets by @arkanaai in: #939 (comment) they were addressed in commit: 557ccad |
|
Thanks. The changes in 557ccad look good:
One small thing: No blockers from me on this PR. |
🔍 Arkana Review — Commit
|
|
Followup review (new commit: Clean removal of the orphaned default variables ( No issues — this is a straightforward cleanup that aligns |
🔍 Arkana PR Review —
|
🔍 Arkana PR Review —
|
bitcoin-coder-bob
force-pushed
the
bob/settings-domain-and-repo
branch
from
e2706bf to
b21375a
Compare
🔍 Iterative Review — Partial Update Support (
|
🔍 Review — Settings Domain (iterative)New commits since last review. Focused on the delta. Architecture & Design ✅Clean domain-driven approach: Security ✅
Concern — Zero-value merge semantics for sentinel fields
With the current merge logic, a caller cannot set these back to 0 via a partial update — the zero will be silently replaced by the current value. Two options:
This is a usability issue, not a security issue, but worth noting in the API docs. Minor nits
Cross-repo impactThe removed env vars ( Tests are thorough — domain validation, admin service CRUD, partial updates, clear idempotency, and repo-level tests across all backends. |
🔍 Review —
|
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Arkana Review — Iterative (commits 1fc333e, 2f660ca)
Delta since last review: Remove unused defaultAllowCSVBlockType config var + seed DB-backed settings from env vars on first boot.
🔴 CRITICAL — Env-var seeded defaults bypass domain.Settings.Validate()
config.go:515-518 — loadSettings() calls defaultSettings() → Upsert() without calling settings.Validate() first. The admin UpdateSettings API correctly calls Validate() before persisting, but the first-boot seed path does not.
Config.Validate() (called after loadSettings()) checks locktime type consistency and BanDuration >= 1, but misses these domain-level constraints:
| Constraint | Example bad env config |
|---|---|
PublicUnilateralExitDelay >= UnilateralExitDelay |
PUBLIC=5, UNILATERAL=100 → persisted, fund safety issue |
BoardingExitDelay != UnilateralExitDelay |
Both set to 86400 → persisted, identical script paths |
RoundMaxParticipantsCount >= RoundMinParticipantsCount |
MIN=128, MAX=1 → persisted |
CheckpointExitDelay > 0 |
Set to 0 → persisted |
| uint32 overflow guards | Values > 2^32 → persisted |
| Satoshi amount bounds | VtxoMinAmount=99999999999 → persisted |
Fix: Add settings.Validate() before the Upsert call:
if settings == nil {
settings = c.defaultSettings()
if err := settings.Validate(); err != nil {
return fmt.Errorf("invalid default settings: %w", err)
}
if err := c.repo.Settings().Upsert(ctx, *settings); err != nil {
return fmt.Errorf("failed to seed default settings: %w", err)
}
}This is protocol-critical — invalid exit delays affect VTXO script safety and forfeit path timing. 🚨 Requires human review.
🟡 E2E test behavioral change — exit delay values differ
The removed updateSettings() helper used unilateral_exit_delay: 512 and boarding_exit_delay: 1024. The new docker-compose env vars use 20 and 30 respectively. Since the scheduler is block type and these are block counts, the new values are valid and arguably more appropriate for e2e tests (shorter blocks = faster tests). But it's a behavioral change — any e2e test that depends on specific delay arithmetic (e.g., waiting for a certain number of blocks for an exit) will see different behavior. Verify the full e2e suite passes with these new values.
🟢 Good
- First-boot env var seeding is the right UX — operators can configure via env vars on initial deploy without needing a second admin API call.
viper.IsSet()guards correctly distinguish "env var explicitly set" from "use scheduler-aware default." The two scheduler-dependent fields (VtxoTreeExpiry,CheckpointExitDelay) fall through to inline defaults when no env var is set.- Comment quality is good — the doc comment on
defaultSettings()clearly explains the first-boot-only semantics. - Removing
updateSettings()from e2e simplifies test setup and makes tests more representative of real deployments.
Summary
One blocker: call settings.Validate() on seeded defaults before persisting (security-critical for exit delays). Everything else in this delta is solid.
Reviewer: Arkana (automated)
Requires: Human sign-off (protocol-critical: exit delay handling on first boot)
i added the Per the 512/1024 seconds the change i made was intentional to remove the out of band |
There was a problem hiding this comment.
🔍 Arkana Review — Commit 29e4f2b (validate seeded defaults)
Delta since last review: Single commit addressing the critical finding from my previous review.
✅ Fix confirmed — config.go:519-522
loadSettings() now calls settings.Validate() before Upsert() on the first-boot seed path. This closes the gap where a bad env-var combination (e.g. PublicUnilateral < Unilateral, RoundMin > RoundMax) could be silently persisted to the DB without validation.
The fix is correct:
- Same
Validate()call used by the adminUpdateSettingspath - Error is returned with descriptive wrapping (
"invalid default settings: %w") - Placement is right: after
defaultSettings(), beforeUpsert()
No new findings. All prior critical/high issues have been addressed across the review iterations.
Reviewer: Arkana (automated)
There was a problem hiding this comment.
🔍 Arkana Review — Iterative (commits 0f734ec, 5142125, ece9dd1)
Reviewer: Arkana (automated)
Changes since last review (29e4f2b):
- Two merge-master commits (conflict resolution)
5142125: Removes 3 unused default constants (defaultRoundMaxParticipantsCount,defaultRoundMinParticipantsCount,defaultVtxoNoCsvValidationCutoffDate) that leaked in from the merge
Assessment: Clean housekeeping. Verified via git grep that the removed constants have zero remaining references on this branch. No new logic, no new risk surface.
Status: No new issues. My previous approvals and protocol-critical flag (exit delay handling requiring human sign-off) still stand.
1 participant
closes #934
variables and into the database
Summary by CodeRabbit
New Features
Documentation
Chores
Tests