Conversation
📝 WalkthroughWalkthroughThis pull request systematically renames all "Delegator" terminology to "Delegate" across the codebase, including gRPC service definitions, protobuf messages, configuration structures, application services, domain models, database repositories, and UI components. The renaming also updates related field names (e.g., Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 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 |
🔍 Arkana Review —
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/interface/grpc/config.go (1)
31-38:⚠️ Potential issue | 🔴 CriticalAdd a
DelegatePort > 0guard around delegate listener/gateway setup inservice.go.The
Validate()method guards the bind check withif c.DelegatePort > 0(lines 31–38), but the delegate server creation inservice.go(starting at line 215) is only guarded byif delegateSvc != nil. This allows the delegate listener to be created on port:0and the gateway to connect to127.0.0.1:0even whenDelegatePort == 0, creating a random listener or unusable gateway target.Update the condition at line 215 to:
if delegateSvc != nil && cfg.DelegatePort > 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interface/grpc/config.go` around lines 31 - 38, The delegate listener/gateway creation in service.go should only run when a fixed delegate port is configured: update the guard around the delegate server creation from just checking delegateSvc to also verify cfg.DelegatePort > 0 (i.e., change the condition to if delegateSvc != nil && cfg.DelegatePort > 0) so the code that creates the delegate listener/gateway (the block that uses delegateSvc to create the listener/connector) is skipped when DelegatePort == 0; locate the delegate setup block using the symbol delegateSvc and the cfg.DelegatePort field and apply this additional check.
🧹 Nitpick comments (2)
internal/config/permissions_test.go (1)
49-52: Consider subtests per method for clearer failures.Wrapping each method assertion in
t.Run(method, ...)will make failures more diagnosable.♻️ Proposed refactor
perms := config.WhitelistedByMethod() for _, method := range allMethods { - _, ok := perms[method] - require.True(t, ok, "missing permission for %s", method) + method := method + t.Run(method, func(t *testing.T) { + _, ok := perms[method] + require.True(t, ok, "missing permission for %s", method) + }) }As per coding guidelines, "
**/*_test.go: Write table-driven tests when possible and use subtests for clarity in Go testing".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/permissions_test.go` around lines 49 - 52, The loop that asserts each method exists in perms should be converted to per-method subtests for clearer failures: replace the for _, method := range allMethods { ... } block with a t.Run(method, func(t *testing.T) { method := method; _, ok := perms[method]; require.True(t, ok, "missing permission for %s", method) }) so each method runs as a distinct subtest and capture the loop variable (method := method) inside the closure to avoid range variable capture issues.internal/test/e2e/delegate_test.go (1)
65-90: Finish the remaining local terminology cleanup in these tests.The RPC/client rename is done, but helpers like
aliceDelegatorClosureanddelegatorVtxoScriptstill use the old term in several scenarios. A small follow-up rename would make the flows easier to scan and avoid mixing both vocabularies in the same file.Also applies to: 315-340, 572-597, 839-863, 1138-1163, 1445-1470
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/test/e2e/delegate_test.go` around lines 65 - 90, Rename the remaining "delegator" local identifiers to use the new terminology (e.g. "delegate") so the test file consistently uses the RPC/client rename; specifically rename aliceDelegatorClosure to aliceDelegateClosure and delegatorVtxoScript to delegateVtxoScript (and update any related variable usages such as vtxoTapKey, vtxoTapTree, and calls like delegatorVtxoScript.TapTree()) and apply the same rename pattern in the other occurrences mentioned (lines ~315-340, 572-597, 839-863, 1138-1163, 1445-1470) to avoid mixing vocabularies.
🤖 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/delegator_batch_handler.go`:
- Around line 40-49: Move the deletion of in-memory intent hashes to after the
repository write succeeds: build taskIds from h.selectedTasks as before, call
repo := h.delegate.svc.dbSvc.Delegate() and invoke repo.CompleteTasks(ctx,
event.Txid, taskIds...) first, check for an error, and only if nil acquire
h.delegate.intentsMtx and delete h.delegate.registeredIntents entries using
selectedTask.intentIDHash(); do not remove registeredIntents before calling
CompleteTasks to avoid losing context on repository failure.
In `@internal/infrastructure/db/badger/delegate_repo.go`:
- Around line 153-156: In GetPendingTaskIDsByInputs, don't silently skip
malformed InputJSON: replace the current json.Unmarshal error handling for
outpointJSON (when parsing data.InputJSON) so that instead of continue it
returns an error that includes the offending task's ID and the underlying
unmarshal error; reference the outpointJSON type and the data.InputJSON field
and construct a descriptive error (e.g., "failed to decode pending task input
for task <taskID>: <err>") so callers can surface the bad row rather than
letting the task become invisible.
---
Outside diff comments:
In `@internal/interface/grpc/config.go`:
- Around line 31-38: The delegate listener/gateway creation in service.go should
only run when a fixed delegate port is configured: update the guard around the
delegate server creation from just checking delegateSvc to also verify
cfg.DelegatePort > 0 (i.e., change the condition to if delegateSvc != nil &&
cfg.DelegatePort > 0) so the code that creates the delegate listener/gateway
(the block that uses delegateSvc to create the listener/connector) is skipped
when DelegatePort == 0; locate the delegate setup block using the symbol
delegateSvc and the cfg.DelegatePort field and apply this additional check.
---
Nitpick comments:
In `@internal/config/permissions_test.go`:
- Around line 49-52: The loop that asserts each method exists in perms should be
converted to per-method subtests for clearer failures: replace the for _, method
:= range allMethods { ... } block with a t.Run(method, func(t *testing.T) {
method := method; _, ok := perms[method]; require.True(t, ok, "missing
permission for %s", method) }) so each method runs as a distinct subtest and
capture the loop variable (method := method) inside the closure to avoid range
variable capture issues.
In `@internal/test/e2e/delegate_test.go`:
- Around line 65-90: Rename the remaining "delegator" local identifiers to use
the new terminology (e.g. "delegate") so the test file consistently uses the
RPC/client rename; specifically rename aliceDelegatorClosure to
aliceDelegateClosure and delegatorVtxoScript to delegateVtxoScript (and update
any related variable usages such as vtxoTapKey, vtxoTapTree, and calls like
delegatorVtxoScript.TapTree()) and apply the same rename pattern in the other
occurrences mentioned (lines ~315-340, 572-597, 839-863, 1138-1163, 1445-1470)
to avoid mixing vocabularies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82d645cb-7f6b-453c-ac87-6117b622dcf4
⛔ Files ignored due to path filters (8)
api-spec/protobuf/gen/go/fulmine/v1/delegate.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/go/fulmine/v1/delegate.pb.gw.gois excluded by!**/*.pb.gw.go,!**/gen/**api-spec/protobuf/gen/go/fulmine/v1/delegate_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/go/fulmine/v1/delegator.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/go/fulmine/v1/delegator.pb.gw.gois excluded by!**/*.pb.gw.go,!**/gen/**api-spec/protobuf/gen/go/fulmine/v1/delegator_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/go/fulmine/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/go/fulmine/v1/service_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (32)
api-spec/openapi/swagger/fulmine/v1/delegate.swagger.jsonapi-spec/openapi/swagger/fulmine/v1/service.swagger.jsonapi-spec/protobuf/fulmine/v1/delegate.protoapi-spec/protobuf/fulmine/v1/service.protocmd/fulmine/main.goenvs/dev.2.envenvs/dev.envinternal/config/config.gointernal/config/permissions.gointernal/config/permissions_test.gointernal/core/application/delegator_batch_handler.gointernal/core/application/delegator_service.gointernal/core/application/service.gointernal/core/application/utils.gointernal/core/domain/delegate.gointernal/infrastructure/db/badger/delegate_repo.gointernal/infrastructure/db/db_test.gointernal/infrastructure/db/sqlite/delegate_repo.gointernal/interface/grpc/config.gointernal/interface/grpc/handlers/delegate_handler.gointernal/interface/grpc/handlers/utils.gointernal/interface/grpc/service.gointernal/interface/web/handlers.gointernal/interface/web/server.gointernal/interface/web/templates/modals/delegate_task.templinternal/interface/web/templates/modals/delegate_task_templ.gointernal/interface/web/templates/pages/delegate.templinternal/interface/web/templates/pages/delegate_templ.gointernal/interface/web/types/delegate.gointernal/test/e2e/delegate_test.gointernal/test/e2e/utils_test.gotest.docker-compose.yml
| repo := h.delegate.svc.dbSvc.Delegate() | ||
| taskIds := make([]string, 0, len(h.selectedTasks)) | ||
| for _, selectedTask := range h.selectedTasks { | ||
| taskIds = append(taskIds, selectedTask.taskID) | ||
| h.delegator.intentsMtx.Lock() | ||
| delete(h.delegator.registeredIntents, selectedTask.intentIDHash()) | ||
| h.delegator.intentsMtx.Unlock() | ||
| h.delegate.intentsMtx.Lock() | ||
| delete(h.delegate.registeredIntents, selectedTask.intentIDHash()) | ||
| h.delegate.intentsMtx.Unlock() | ||
| } | ||
|
|
||
| return repo.CompleteTasks(ctx, event.Txid, taskIds...) |
There was a problem hiding this comment.
Persist completion before clearing registeredIntents.
The in-memory mapping is removed before repo.CompleteTasks(...) runs. If that write fails, the task stays pending in storage but the service has already forgotten the intent hash, which makes retry/recovery lose context. Delete the entries only after the repository update succeeds.
Proposed fix
func (h *delegateBatchSessionHandler) OnBatchFinalized(
ctx context.Context, event client.BatchFinalizedEvent,
) error {
repo := h.delegate.svc.dbSvc.Delegate()
taskIds := make([]string, 0, len(h.selectedTasks))
for _, selectedTask := range h.selectedTasks {
taskIds = append(taskIds, selectedTask.taskID)
- h.delegate.intentsMtx.Lock()
- delete(h.delegate.registeredIntents, selectedTask.intentIDHash())
- h.delegate.intentsMtx.Unlock()
}
- return repo.CompleteTasks(ctx, event.Txid, taskIds...)
+ if err := repo.CompleteTasks(ctx, event.Txid, taskIds...); err != nil {
+ return err
+ }
+ for _, selectedTask := range h.selectedTasks {
+ h.delegate.intentsMtx.Lock()
+ delete(h.delegate.registeredIntents, selectedTask.intentIDHash())
+ h.delegate.intentsMtx.Unlock()
+ }
+ return nil
}📝 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.
| repo := h.delegate.svc.dbSvc.Delegate() | |
| taskIds := make([]string, 0, len(h.selectedTasks)) | |
| for _, selectedTask := range h.selectedTasks { | |
| taskIds = append(taskIds, selectedTask.taskID) | |
| h.delegator.intentsMtx.Lock() | |
| delete(h.delegator.registeredIntents, selectedTask.intentIDHash()) | |
| h.delegator.intentsMtx.Unlock() | |
| h.delegate.intentsMtx.Lock() | |
| delete(h.delegate.registeredIntents, selectedTask.intentIDHash()) | |
| h.delegate.intentsMtx.Unlock() | |
| } | |
| return repo.CompleteTasks(ctx, event.Txid, taskIds...) | |
| repo := h.delegate.svc.dbSvc.Delegate() | |
| taskIds := make([]string, 0, len(h.selectedTasks)) | |
| for _, selectedTask := range h.selectedTasks { | |
| taskIds = append(taskIds, selectedTask.taskID) | |
| } | |
| if err := repo.CompleteTasks(ctx, event.Txid, taskIds...); err != nil { | |
| return err | |
| } | |
| for _, selectedTask := range h.selectedTasks { | |
| h.delegate.intentsMtx.Lock() | |
| delete(h.delegate.registeredIntents, selectedTask.intentIDHash()) | |
| h.delegate.intentsMtx.Unlock() | |
| } | |
| return nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/application/delegator_batch_handler.go` around lines 40 - 49,
Move the deletion of in-memory intent hashes to after the repository write
succeeds: build taskIds from h.selectedTasks as before, call repo :=
h.delegate.svc.dbSvc.Delegate() and invoke repo.CompleteTasks(ctx, event.Txid,
taskIds...) first, check for an error, and only if nil acquire
h.delegate.intentsMtx and delete h.delegate.registeredIntents entries using
selectedTask.intentIDHash(); do not remove registeredIntents before calling
CompleteTasks to avoid losing context on repository failure.
| var opJSONs []outpointJSON | ||
| if err := json.Unmarshal([]byte(data.InputJSON), &opJSONs); err != nil { | ||
| continue | ||
| continue | ||
| } |
There was a problem hiding this comment.
Don't silently skip malformed pending-task inputs.
GetPendingTaskIDsByInputs is the same-input guard. If a pending row has bad InputJSON, continue makes that task invisible and can let another task reuse the same outpoint. Return the decode failure with the task ID instead of ignoring it.
Proposed fix
var opJSONs []outpointJSON
if err := json.Unmarshal([]byte(data.InputJSON), &opJSONs); err != nil {
- continue
+ return nil, fmt.Errorf(
+ "failed to decode inputs for pending delegate task %s: %w",
+ data.ID, err,
+ )
}As per coding guidelines, "Always check errors and use descriptive error messages in error handling".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/infrastructure/db/badger/delegate_repo.go` around lines 153 - 156,
In GetPendingTaskIDsByInputs, don't silently skip malformed InputJSON: replace
the current json.Unmarshal error handling for outpointJSON (when parsing
data.InputJSON) so that instead of continue it returns an error that includes
the offending task's ID and the underlying unmarshal error; reference the
outpointJSON type and the data.InputJSON field and construct a descriptive error
(e.g., "failed to decode pending task input for task <taskID>: <err>") so
callers can surface the bad row rather than letting the task become invisible.
1 participant
This contains changes for the migration to the right naming of the delegate service (delegator -> delegate).
Includes breaking changes to grpc and env vars:
FULMINE_DELEGATOR_*->FULMINE_DELEGATE_*The changes are non-breaking for REST clients:
/v1/delegate/infoand also to old/v1/delegator/info, and the endpoint returns bothdelegateAddressand olddelegatorAddress.Changes to service.proto are deliberately breaking because that's considered admin interface.
Please @sekulicd @Kukks review
Summary by CodeRabbit
Release Notes
API Updates
/v1/delegate/infoendpoint now available (replaces previous naming)User Interface
Configuration
FULMINE_DELEGATE_ENABLED)