Open
Conversation
- Add ProviderOrderToken score + last_order_assigned_at; score history + assignment run tables - Wire PaymentOrder edges with ON DELETE CASCADE; optional assignment market rate snapshots - Use Ent default FK column names (no edge Field()); legacy provision bucket as StorageKey - Migration: new tables, payment_orders columns, provision_buckets FK relaxations - Fix assignment tests: filter runs with HasPaymentOrderWith(paymentorder.IDEQ(...))
Contributor
|
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:
📝 WalkthroughWalkthroughReplaces bucket-based provider routing with a DB-driven, score-based assignment system: adds ProviderAssignmentRun and ProviderOrderTokenScoreHistory schemas and services/assignment package; removes provision-bucket edges/usages; resolves fiat currency from institution; and wires scoring side-effects into provider flows and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant AssignSvc as assignment.Service
participant DB as Database
participant Redis as Redis
participant Provider as ProviderHost
Caller->>AssignSvc: AssignPaymentOrder(fields)
Activate AssignSvc
AssignSvc->>DB: Load PaymentOrder + Token + Institution (snapshot market rates)
DB-->>AssignSvc: PaymentOrder with market snapshot
AssignSvc->>DB: Query ProviderOrderTokens (filter by currency/token/network, balance, visibility)
DB-->>AssignSvc: Candidate tokens
AssignSvc->>DB: Record ProviderAssignmentRun (attempt, snapshot)
alt candidate found
AssignSvc->>DB: Reserve fiat balance (transaction)
DB-->>AssignSvc: OK
AssignSvc->>Redis: Set order_request_{id} hash & meta
Redis-->>AssignSvc: OK
AssignSvc->>Provider: POST /new_order (notify with HMAC)
alt notify OK
Provider-->>AssignSvc: 200
AssignSvc->>DB: Persist assignment, update last_order_assigned_at
else notify fails
Provider-->>AssignSvc: error
AssignSvc->>Redis: Release balance & cleanup keys
AssignSvc->>DB: Update ProviderAssignmentRun with error
end
else no candidate
AssignSvc->>AssignSvc: TryFallbackAssignment (similar flow)
end
Deactivate AssignSvc
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e93bc0161
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ent/migrate/migrations/20260323233838_provider_selection_phase1.sql
Outdated
Show resolved
Hide resolved
![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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
controllers/accounts/profile.go (1)
667-729:⚠️ Potential issue | 🟠 MajorToken updates are only upserts, not full syncs
This method prunes omitted fiat accounts below, but it never does the same for
provider_order_tokens. If a provider removes or replaces a token/network config, the old row survives and can still be selected/scored now that routing is driven fromprovider_order_tokens. If full-replacement semantics are intended, mirror the kept-set/delete pass you already use for fiat accounts.Also applies to: 775-845
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/accounts/profile.go` around lines 667 - 729, The loop over tokenOperations only upserts tokens and never deletes provider_order_tokens that were omitted; add a pass that collects kept token IDs (e.g., when op.IsUpdate use op.ExistingToken.ID and when creating capture the new ID) and after the loop run a delete query via tx.ProviderOrderToken.Delete().Where(providerID == provider.ID, currencyID == op.Currency.ID (or payload.Currency), ID NOT IN keptIDs) to remove any rows not in the kept set, mirroring the fiat-account prune logic; ensure this runs inside the same transaction (tx) and handle rollback/logging on error.controllers/provider/provider.go (1)
1455-1497:⚠️ Potential issue | 🔴 CriticalCommit
txbefore score/webhook/settlement side effects.This success path has already updated fulfillment, order, logs, and balances inside
tx, but there is notx.Commit()before Lines 1455-1497. That leaves the transaction open while score-history writes, Redis cleanup, webhook delivery, and settlement submission are already happening.🛠️ Minimal fix
+ if err := tx.Commit(); err != nil { + logger.WithFields(logger.Fields{ + "Error": fmt.Sprintf("%v", err), + "Trx Id": payload.TxID, + "Network": fulfillment.Edges.Order.Edges.Token.Edges.Network.Identifier, + }).Errorf("Failed to commit fulfilled order transaction") + u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to update order status", nil) + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/provider/provider.go` around lines 1455 - 1497, The code performs side effects (assignment.ApplyProviderScoreChange, storage.RedisClient.Del, u.SendPaymentOrderWebhook, and the settlement goroutine using orderService.NewOrderTron/NewOrderStarknet/NewOrderEVM) before committing the DB transaction; move a tx.Commit() (or tx.Commit(ctx) depending on your tx API) to immediately before those lines (before calling assignment.ApplyProviderScoreChange and before spawning the settlement goroutine), check/handle the commit error (log and abort further side effects if commit fails), and only proceed with Redis cleanup, webhook sending, score changes, and the async settlement after a successful commit; reference the existing tx variable and the functions assignment.ApplyProviderScoreChange, storage.RedisClient.Del, u.SendPaymentOrderWebhook, and the settlement goroutine creation to locate where to insert the commit and error handling.ent/providerordertoken_update.go (1)
385-478:⚠️ Potential issue | 🟠 MajorMake child edges immutable to prevent reparenting of audit rows.
The
score_historiesandassignment_runsedges in the child schemas are currently mutable. Add.Immutable()to theprovider_order_tokenedge in bothent/schema/providerordertokenscorehistory.goandent/schema/providerassignmentrun.go, then runmake gen-entto eliminate theAdd*,Remove*, andClear*mutators from the parent update builders (lines 385–478, 725–814, 1182–1275, 1552–1641 in the generated file).Without this change, callers can detach or move audit records after creation, breaking the append-only guarantee.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ent/providerordertoken_update.go` around lines 385 - 478, The child edges score_histories and assignment_runs are currently mutable which allows reparenting audit rows; open ent/schema/providerordertokenscorehistory.go and ent/schema/providerassignmentrun.go and add .Immutable() to the provider_order_token edge definition (the provider_order_token Edge on ProviderOrderTokenScoreHistory and ProviderAssignmentRun), then run make gen-ent to regenerate the ent code so the parent update builder (ProviderOrderTokenUpdate) no longer contains Add*/Remove*/Clear* mutators for those edges.ent/paymentorder_update.go (1)
892-1033:⚠️ Potential issue | 🟠 MajorAdd
.Immutable()to audit/history relation edges inent/schema.The reverse edges (from
ProviderAssignmentRunandProviderOrderTokenScoreHistoryback toPaymentOrder) and forward edges (fromPaymentOrderto these relations) lack.Immutable()markers. This allows generated update builders to exposeAdd*,Remove*, andClear*methods for these relations, which violates append-only semantics and weakens the score-history idempotency boundary.Add
.Immutable()to:
ent/schema/paymentorder.go: edges forprovider_assignment_runsandprovider_order_token_score_historiesent/schema/providerassignmentrun.go: reverse edge forpayment_orderent/schema/providerordertokenscorehistory.go: reverse edge forpayment_orderThen run
make gen-entto regenerate the update builders without mutable edge operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ent/paymentorder_update.go` around lines 892 - 1033, The generated update builders expose mutating edge methods (Add*/Remove*/Clear*) for the provider_assignment_runs and provider_order_token_score_histories relations; to fix this, mark those edges as immutable in the ent schema: add .Immutable() to the provider_assignment_runs and provider_order_token_score_histories edges in the PaymentOrder schema (ent/schema/paymentorder.go) and to the reverse payment_order edges in ProviderAssignmentRun (ent/schema/providerassignmentrun.go) and ProviderOrderTokenScoreHistory (ent/schema/providerordertokenscorehistory.go), then run make gen-ent to regenerate so methods like AddProviderAssignmentRunIDs, RemoveProviderOrderTokenScoreHistoryIDs, RemoveProviderAssignmentRuns, ClearProviderOrderTokenScoreHistories, etc. are removed from the update builder.
🧹 Nitpick comments (5)
ent/provisionbucket/provisionbucket.go (1)
22-35: Audit downstream SQL for thisprovision_bucketsFK rename.This generated field now maps to
fiat_currency_provision_buckets, notfiat_currency_id. Any manual SQL, BI queries, or dashboards still joining on the old column name will break once the migration is deployed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ent/provisionbucket/provisionbucket.go` around lines 22 - 35, The generated constant FieldFiatCurrencyID now points to the renamed DB column "fiat_currency_provision_buckets" (in the provision_buckets table) which will break any downstream SQL/BI/dashboards expecting "fiat_currency_id"; search for usages of FieldFiatCurrencyID, Table, Columns and any hardcoded "fiat_currency_id" references across the repo/ETL/BI scripts and update those joins/queries to use the new column name (or restore the old column alias in the migration if preferred), and add a short comment near FieldFiatCurrencyID documenting the rename so future readers know the column changed.ent/schema/providerassignmentrun.go (1)
32-41: Add indexes on the audit FKs before this table starts growing.
provider_assignment_runsis append-only and the service/tests already query it by payment order. Without indexes onpayment_orderandprovider_order_token, those lookups and cascade deletes will degrade into table scans as volume grows.📌 Suggested schema update
+ "entgo.io/ent/schema/index"+func (ProviderAssignmentRun) Indexes() []ent.Index { + return []ent.Index{ + index.Edges("payment_order"), + index.Edges("provider_order_token"), + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ent/schema/providerassignmentrun.go` around lines 32 - 41, The ProviderAssignmentRun schema lacks DB indexes on the foreign keys used for lookups/deletes; add an Indexes() implementation to the ProviderAssignmentRun ent.Schema that creates indexes on the generated FK columns for "payment_order" and "provider_order_token" (e.g. payment_order_id and provider_order_token_id) so queries using the edge.From relations and cascade deletes do not degenerate into table scans; reference the ProviderAssignmentRun type, its Edges() relationships, and the FK column names when adding the index definitions (and add a migration to apply them).controllers/provider/provider.go (1)
525-529: Avoid the extra institution lookup per CSV row.The prefetch pass above already walks the unique institution codes, but Lines 526-529 add another
GetInstitutionByCodecall for every exported order. With the 10k export cap, that becomes an avoidable N+1 query pattern. Cache the currency code alongside the institution name during the prefetch step.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/provider/provider.go` around lines 525 - 529, The code is performing a redundant GetInstitutionByCode lookup per exported paymentOrder causing an N+1 query; modify the prefetch pass that currently collects unique institution codes to also store each institution's fiat currency code (e.g., in a map[string]string keyed by institution code), then in the export loop replace the inline GetInstitutionByCode call (inside the paymentOrder handling block) with a lookup into that prefetch map to set currencyCode; ensure you still handle missing keys/null currencies safely (default empty string) so no extra DB calls are needed per row.services/assignment/scoring_test.go (1)
383-390: Unused helper function.
getProviderPOTis defined but never called in this file. Consider removing it to avoid dead code, or if it's intended for future use, add a test case that uses it.🔧 Remove unused function
-func getProviderPOT(t *testing.T, ctx context.Context, provID string) *ent.ProviderOrderToken { - t.Helper() - pot, err := db.Client.ProviderOrderToken.Query(). - Where(providerordertoken.HasProviderWith(providerprofile.IDEQ(provID))). - Only(ctx) - require.NoError(t, err) - return pot -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/assignment/scoring_test.go` around lines 383 - 390, The helper getProviderPOT is unused; either remove it or add a test that exercises it. If removing, delete the getProviderPOT function definition (the function that calls db.Client.ProviderOrderToken.Query().Where(providerordertoken.HasProviderWith(providerprofile.IDEQ(provID))).Only(ctx) and uses require.NoError). If keeping, add a new test that calls getProviderPOT with a valid provID (ensuring a ProviderOrderToken exists in test setup) and asserts the returned *ent.ProviderOrderToken is non-nil; reference the function name getProviderPOT and the query chain providerordertoken.HasProviderWith/providerprofile.IDEQ to locate the code.ent/providerassignmentrun_update.go (1)
185-223: Make thepayment_orderrelation create-only.An assignment-run row is the audit record for a specific payment order, so allowing
SetPaymentOrderID(...)orClearPaymentOrder()after insert lets the row be reparented or detached. Please enforce this relation as fixed once the run is created.Based on learnings Only edit
ent/schema/files; never edit generated files underent/. After schema changes, runmake gen-entand add/apply migrations.Also applies to: 336-364, 564-603, 745-773
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ent/providerassignmentrun_update.go` around lines 185 - 223, The payment_order edge on ProviderAssignmentRun must be create-only; remove/disable generated mutators SetPaymentOrderID and ClearPaymentOrder by making the relation immutable in the schema: update the ProviderAssignmentRun schema (the ent/schema ProviderAssignmentRun type) to mark the edge to PaymentOrder as immutable/create-only (e.g., use Edge/Field options such as immutable or use hooks/annotations supported by your ent setup to prevent updates), then re-run make gen-ent and add/apply the generated migration; after regenerating, confirm SetPaymentOrderID and ClearPaymentOrder are no longer emitted (also apply the same schema change for the other referenced places noted: the other ProviderAssignmentRun schema occurrences) so the relation cannot be changed after insert.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controllers/provider/provider.go`:
- Around line 2023-2025: The call to assignment.ApplyProviderScoreChange is
using the Gin handler context variable ctx but it performs I/O and should use
the request context variable reqCtx; update the call to pass reqCtx instead of
ctx (keep orderID, assignment.ScoreEventFulfilledValidated, and
decimal.NewFromFloat(assignment.RewardFulfilledValidated) unchanged) and
preserve the existing error logging (logger.WithFields...Warnf) so only the
context argument is switched to reqCtx.
In `@ent/migrate/migrations/20260323233838_provider_selection_phase1.sql`:
- Around line 5-6: The migration is adding "score" to table
provider_order_tokens as NOT NULL which will fail on populated tables; change
the migration to add "score" as nullable (or add a DEFAULT if 0 is acceptable)
and then perform a backfill plus a separate migration to ALTER TABLE ... ALTER
COLUMN score SET NOT NULL; keep "last_order_assigned_at" as NULLABLE as-is.
Specifically, modify the ALTER TABLE in the migration to add "score" without NOT
NULL (or with DEFAULT), run a one-off backfill updating
provider_order_tokens.score for existing rows, and create a follow-up migration
to set NOT NULL on provider_order_tokens.score once backfill completes.
- Around line 7-38: Add non-unique indexes on the foreign-key columns to avoid
table scans during CASCADE deletes: create an index on
provider_assignment_runs(payment_order_provider_assignment_runs) and another on
provider_assignment_runs(provider_order_token_assignment_runs), and create
indexes on
provider_order_token_score_histories(payment_order_provider_order_token_score_histories)
and provider_order_token_score_histories(provider_order_token_score_histories)
so the FK constraints (referenced by constraints
provider_assignment_runs_payme_84d7fae6... and
provider_assignment_runs_provider_order_tokens_assignment_runs and
provider_order_token_score_his_050ed76d... and
provider_order_token_score_his_86d4d0f0...) can use leading-column indexes for
efficient deletes and lookups.
In `@ent/providerordertokenscorehistory_update.go`:
- Around line 41-112: The generated update builders (e.g., SetEventType,
SetDelta, SetPaymentOrderID, SetProviderOrderTokenID) allow mutating append-only
history; add .Immutable() to the schema field and edge definitions for
event_type and delta and for the payment_order and provider_order_token edges in
ent/schema/providerordertokenscorehistory.go so the code generator emits
immutable behavior, then run make gen-ent to regenerate the updated builders.
In `@ent/schema/providerordertokenscorehistory.go`:
- Around line 18-46: The ProviderOrderTokenScoreHistory schema currently uses
TimeMixin which adds an updated_at UpdateDefault allowing updates; to enforce
append-only semantics, add a Hooks() implementation on
ProviderOrderTokenScoreHistory that rejects Update/UpdateOne/UpdateOneID
mutations (or validate in a mutation hook) and/or mark mutable fields as
Immutable() (e.g., make delta, event_type and id immutable) so records can only
be created; implement the hook in the ProviderOrderTokenScoreHistory schema type
(ProviderOrderTokenScoreHistory.Hooks) to return an ent.Hook that inspects the
mutation operation and returns an error for update operations.
In `@services/assignment/assign_test.go`:
- Around line 40-49: This test sets package globals db.Client, db.DB, and
db.RedisClient to in-memory fixtures but only closes them in cleanup; restore
the original globals after the test to avoid leaking closed instances into other
tests. Before replacing them, capture the previous values (originalClient,
originalDB, originalRedis) and register a t.Cleanup that closes the test
fixtures and assigns db.Client = originalClient, db.DB = originalDB, and
db.RedisClient = originalRedis; ensure cleanup still calls mr.Close() and
rdb.Close() and that client.Schema.Create errors are handled as before.
In `@services/assignment/assign.go`:
- Around line 528-569: The duplicate-order race happens because
ReserveFiatBalance (s.balanceService.ReserveFiatBalance) is called before
checking Redis for an existing order request (orderKey via
storage.RedisClient.Exists / HGet), so a second caller can reserve funds then
detect the duplicate and still commit the tx; to fix, perform the Redis
existence check (using storage.RedisClient.Exists and HGet for "providerId")
before calling s.balanceService.ReserveFiatBalance, and only call
ReserveFiatBalance and proceed with tx.Commit when the order request does not
already exist for the same provider; alternatively, if you must preserve current
ordering, add a safe pre-check and abort the balance reservation when
existingProviderID == order.ProviderID to avoid double reservation.
- Around line 214-219: The code currently returns an error if updating
fallback_tried_at fails after sendOrderRequest already succeeded; instead,
change the Save(ctx) error handling for
storage.Client.PaymentOrder.UpdateOneID(...).SetFallbackTriedAt(...).SetOrderPercent(...)
so that on setErr you log the failure with logger.WithFields(...) but do NOT
return fmt.Errorf — allow execution to continue (i.e., treat persistence failure
as non-fatal) so the successful sendOrderRequest is not reported as a failed
fallback; keep the existing log message and consider adding a metric or retry
later, but do not propagate an error upward.
- Around line 661-689: The current flow calls s.notifyProvider(ctx,
orderRequestData) before tx.Commit(), which can notify an external provider for
an order that may never be persisted; change the ordering so the database
transaction is committed successfully before calling s.notifyProvider (i.e.,
call tx.Commit() and check for error first, then call s.notifyProvider), using
the same error-logging and redis cleanup logic if commit fails, and ensure
references to orderRequestData, notifyProvider, tx.Commit, orderKey and metaKey
are updated accordingly; if immediate notify-after-commit is not acceptable,
implement an outbox pattern (persist a pending notification record within the
same transaction and have a separate worker read and call s.notifyProvider) so
notifications only occur for durable transactions.
In `@services/assignment/scoring.go`:
- Around line 124-139: Wrap the history insert and the cumulative score update
in a single database transaction so they commit or roll back together: start a
transaction with storage.Client.Tx(ctx), run
tx.ProviderOrderTokenScoreHistory.Create().SetPaymentOrderID(...).SetProviderOrderTokenID(...).SetEventType(...).SetDelta(...).Save(ctx)
and if that returns ent.IsConstraintError(err) rollback and return nil,
otherwise on any error rollback and return; then run
tx.ProviderOrderToken.UpdateOneID(pot.ID).AddScore(delta).Exec(ctx) and if it
errors rollback and return, and finally commit the transaction. Ensure you use
the tx (not storage.Client) for both ProviderOrderTokenScoreHistory.Create and
ProviderOrderToken.UpdateOneID so the two operations are atomic.
In `@services/assignment/select.go`:
- Around line 72-79: The exclude-list comparison uses
orderConf.ProviderMaxRetryAttempts directly and when that value is 0 it makes
the check always true (excluding everyone); clamp ProviderMaxRetryAttempts to a
minimum of 1 before performing excludeCount comparisons. Update the logic around
countProviderInExcludeList and the checks in the block handling order.OrderType
("otc" vs regular) to use a local minAttempts :=
orderConf.ProviderMaxRetryAttempts; if minAttempts < 1 { minAttempts = 1 } (or
equivalent) and use minAttempts in the >= comparison, and apply the same clamped
value in the candidate-loop checks that reference ProviderMaxRetryAttempts.
In `@services/assignment/service.go`:
- Around line 50-66: The code in service.go currently falls through to
decimal.Zero and returns nil error when no effective rate is found; update the
rate-resolution branch (the switch on side handling RateSideBuy and RateSideSell
that checks tokenConfig.FixedBuyRate / FixedSellRate and
tokenConfig.Edges.Currency.MarketBuyRate / MarketSellRate) to return a non-nil
error if no rate was set (unsupported side, missing fixed rates, or zero market
snapshot), e.g. detect when rate.IsZero() after the switch and return
fmt.Errorf("no effective rate for side=%v token=%s", side, tokenConfig.Symbol)
(add fmt to imports); ensure callers still receive (decimal.Decimal, error).
---
Outside diff comments:
In `@controllers/accounts/profile.go`:
- Around line 667-729: The loop over tokenOperations only upserts tokens and
never deletes provider_order_tokens that were omitted; add a pass that collects
kept token IDs (e.g., when op.IsUpdate use op.ExistingToken.ID and when creating
capture the new ID) and after the loop run a delete query via
tx.ProviderOrderToken.Delete().Where(providerID == provider.ID, currencyID ==
op.Currency.ID (or payload.Currency), ID NOT IN keptIDs) to remove any rows not
in the kept set, mirroring the fiat-account prune logic; ensure this runs inside
the same transaction (tx) and handle rollback/logging on error.
In `@controllers/provider/provider.go`:
- Around line 1455-1497: The code performs side effects
(assignment.ApplyProviderScoreChange, storage.RedisClient.Del,
u.SendPaymentOrderWebhook, and the settlement goroutine using
orderService.NewOrderTron/NewOrderStarknet/NewOrderEVM) before committing the DB
transaction; move a tx.Commit() (or tx.Commit(ctx) depending on your tx API) to
immediately before those lines (before calling
assignment.ApplyProviderScoreChange and before spawning the settlement
goroutine), check/handle the commit error (log and abort further side effects if
commit fails), and only proceed with Redis cleanup, webhook sending, score
changes, and the async settlement after a successful commit; reference the
existing tx variable and the functions assignment.ApplyProviderScoreChange,
storage.RedisClient.Del, u.SendPaymentOrderWebhook, and the settlement goroutine
creation to locate where to insert the commit and error handling.
In `@ent/paymentorder_update.go`:
- Around line 892-1033: The generated update builders expose mutating edge
methods (Add*/Remove*/Clear*) for the provider_assignment_runs and
provider_order_token_score_histories relations; to fix this, mark those edges as
immutable in the ent schema: add .Immutable() to the provider_assignment_runs
and provider_order_token_score_histories edges in the PaymentOrder schema
(ent/schema/paymentorder.go) and to the reverse payment_order edges in
ProviderAssignmentRun (ent/schema/providerassignmentrun.go) and
ProviderOrderTokenScoreHistory (ent/schema/providerordertokenscorehistory.go),
then run make gen-ent to regenerate so methods like AddProviderAssignmentRunIDs,
RemoveProviderOrderTokenScoreHistoryIDs, RemoveProviderAssignmentRuns,
ClearProviderOrderTokenScoreHistories, etc. are removed from the update builder.
In `@ent/providerordertoken_update.go`:
- Around line 385-478: The child edges score_histories and assignment_runs are
currently mutable which allows reparenting audit rows; open
ent/schema/providerordertokenscorehistory.go and
ent/schema/providerassignmentrun.go and add .Immutable() to the
provider_order_token edge definition (the provider_order_token Edge on
ProviderOrderTokenScoreHistory and ProviderAssignmentRun), then run make gen-ent
to regenerate the ent code so the parent update builder
(ProviderOrderTokenUpdate) no longer contains Add*/Remove*/Clear* mutators for
those edges.
---
Nitpick comments:
In `@controllers/provider/provider.go`:
- Around line 525-529: The code is performing a redundant GetInstitutionByCode
lookup per exported paymentOrder causing an N+1 query; modify the prefetch pass
that currently collects unique institution codes to also store each
institution's fiat currency code (e.g., in a map[string]string keyed by
institution code), then in the export loop replace the inline
GetInstitutionByCode call (inside the paymentOrder handling block) with a lookup
into that prefetch map to set currencyCode; ensure you still handle missing
keys/null currencies safely (default empty string) so no extra DB calls are
needed per row.
In `@ent/providerassignmentrun_update.go`:
- Around line 185-223: The payment_order edge on ProviderAssignmentRun must be
create-only; remove/disable generated mutators SetPaymentOrderID and
ClearPaymentOrder by making the relation immutable in the schema: update the
ProviderAssignmentRun schema (the ent/schema ProviderAssignmentRun type) to mark
the edge to PaymentOrder as immutable/create-only (e.g., use Edge/Field options
such as immutable or use hooks/annotations supported by your ent setup to
prevent updates), then re-run make gen-ent and add/apply the generated
migration; after regenerating, confirm SetPaymentOrderID and ClearPaymentOrder
are no longer emitted (also apply the same schema change for the other
referenced places noted: the other ProviderAssignmentRun schema occurrences) so
the relation cannot be changed after insert.
In `@ent/provisionbucket/provisionbucket.go`:
- Around line 22-35: The generated constant FieldFiatCurrencyID now points to
the renamed DB column "fiat_currency_provision_buckets" (in the
provision_buckets table) which will break any downstream SQL/BI/dashboards
expecting "fiat_currency_id"; search for usages of FieldFiatCurrencyID, Table,
Columns and any hardcoded "fiat_currency_id" references across the repo/ETL/BI
scripts and update those joins/queries to use the new column name (or restore
the old column alias in the migration if preferred), and add a short comment
near FieldFiatCurrencyID documenting the rename so future readers know the
column changed.
In `@ent/schema/providerassignmentrun.go`:
- Around line 32-41: The ProviderAssignmentRun schema lacks DB indexes on the
foreign keys used for lookups/deletes; add an Indexes() implementation to the
ProviderAssignmentRun ent.Schema that creates indexes on the generated FK
columns for "payment_order" and "provider_order_token" (e.g. payment_order_id
and provider_order_token_id) so queries using the edge.From relations and
cascade deletes do not degenerate into table scans; reference the
ProviderAssignmentRun type, its Edges() relationships, and the FK column names
when adding the index definitions (and add a migration to apply them).
In `@services/assignment/scoring_test.go`:
- Around line 383-390: The helper getProviderPOT is unused; either remove it or
add a test that exercises it. If removing, delete the getProviderPOT function
definition (the function that calls
db.Client.ProviderOrderToken.Query().Where(providerordertoken.HasProviderWith(providerprofile.IDEQ(provID))).Only(ctx)
and uses require.NoError). If keeping, add a new test that calls getProviderPOT
with a valid provID (ensuring a ProviderOrderToken exists in test setup) and
asserts the returned *ent.ProviderOrderToken is non-nil; reference the function
name getProviderPOT and the query chain
providerordertoken.HasProviderWith/providerprofile.IDEQ to locate the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fbaae33-213a-4b8c-b2e4-403142ad70b7
⛔ Files ignored due to path filters (1)
ent/migrate/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (90)
controllers/accounts/profile.gocontrollers/index.gocontrollers/provider/provider.gocontrollers/provider/provider_test.gocontrollers/sender/sender_test.goent/client.goent/ent.goent/fiatcurrency.goent/fiatcurrency/fiatcurrency.goent/fiatcurrency/where.goent/fiatcurrency_create.goent/fiatcurrency_query.goent/fiatcurrency_update.goent/hook/hook.goent/migrate/migrations/20260323233838_provider_selection_phase1.sqlent/migrate/schema.goent/mutation.goent/paymentorder.goent/paymentorder/paymentorder.goent/paymentorder/where.goent/paymentorder_create.goent/paymentorder_query.goent/paymentorder_update.goent/predicate/predicate.goent/providerassignmentrun.goent/providerassignmentrun/providerassignmentrun.goent/providerassignmentrun/where.goent/providerassignmentrun_create.goent/providerassignmentrun_delete.goent/providerassignmentrun_query.goent/providerassignmentrun_update.goent/providerordertoken.goent/providerordertoken/providerordertoken.goent/providerordertoken/where.goent/providerordertoken_create.goent/providerordertoken_query.goent/providerordertoken_update.goent/providerordertokenscorehistory.goent/providerordertokenscorehistory/providerordertokenscorehistory.goent/providerordertokenscorehistory/where.goent/providerordertokenscorehistory_create.goent/providerordertokenscorehistory_delete.goent/providerordertokenscorehistory_query.goent/providerordertokenscorehistory_update.goent/providerprofile.goent/providerprofile/providerprofile.goent/providerprofile/where.goent/providerprofile_create.goent/providerprofile_query.goent/providerprofile_update.goent/provisionbucket.goent/provisionbucket/provisionbucket.goent/provisionbucket/where.goent/provisionbucket_create.goent/provisionbucket_query.goent/provisionbucket_update.goent/runtime/runtime.goent/schema/fiatcurrency.goent/schema/paymentorder.goent/schema/providerassignmentrun.goent/schema/providerordertoken.goent/schema/providerordertokenscorehistory.goent/schema/providerprofile.goent/schema/provisionbucket.goent/tx.goservices/assignment/assign.goservices/assignment/assign_test.goservices/assignment/constants.goservices/assignment/scoring.goservices/assignment/scoring_test.goservices/assignment/select.goservices/assignment/select_test.goservices/assignment/service.goservices/common/indexer.goservices/common/order.goservices/indexer/evm.goservices/indexer/starknet.goservices/indexer/tron.goservices/order/evm.goservices/priority_queue.goservices/priority_queue_test.goservices/receive_address.gotasks/fulfillments_webhooks.gotasks/indexing.gotasks/order_requests.gotasks/stale_ops.gotasks/startup.gotypes/types.goutils/test/db.goutils/utils.go
💤 Files with no reviewable changes (11)
- ent/schema/providerprofile.go
- ent/schema/fiatcurrency.go
- ent/fiatcurrency/where.go
- ent/fiatcurrency_create.go
- ent/providerprofile/where.go
- controllers/provider/provider_test.go
- ent/providerprofile_create.go
- ent/fiatcurrency/fiatcurrency.go
- ent/providerprofile/providerprofile.go
- ent/fiatcurrency_update.go
- ent/providerprofile_update.go
ent/migrate/migrations/20260323233838_provider_selection_phase1.sql
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/common/order.go (1)
1406-1458:⚠️ Potential issue | 🔴 CriticalBuild failure: return value count mismatch and undefined variables.
The pipeline fails with
too many return valuesat line 1436. This block has several critical issues from an incomplete refactor:
Return value mismatch: Lines 1436-1438 and 1452-1455 return 6 values, but the function signature at line 1248 declares only 5 return values:
(*types.PaymentOrderFields, *ent.Token, *ent.Institution, *ent.FiatCurrency, error).Undefined variable
orderToken: Line 1407 assigns the query result to_(discarded), but lines 1444-1445 referenceorderTokenwhich doesn't exist.Undefined variable
isPrivate: Referenced at lines 1446 and 1450 but never declared in this function scope.Undefined variable
provisionBucket: Referenced at line 1450 but never defined (appears to be remnant from removed provision bucket logic).🐛 Proposed fix: Remove dead code block and fix return statements
The block from lines 1444-1456 references variables that were removed during the provision-bucket elimination. Since
validateAndPreparePaymentOrderDatano longer deals with provision buckets, this entire conditional block should be removed:if paymentOrderFields.ProviderID != "" { - _, err := db.Client.ProviderOrderToken. + orderToken, err := db.Client.ProviderOrderToken. Query(). // ... query ... Only(ctx) if err != nil { if ent.IsNotFound(err) { if cErr := handleCancellationForIndexedOrder(ctx, network, paymentOrderFields.GatewayID, existingOrder, paymentOrderFields, "Provider not available", refundOrder); cErr != nil { - return nil, nil, nil, nil, nil, fmt.Errorf("%s - failed to handle cancellation: %w", paymentOrderFields.GatewayID, cErr) + return nil, nil, nil, nil, fmt.Errorf("%s - failed to handle cancellation: %w", paymentOrderFields.GatewayID, cErr) } - return nil, nil, nil, nil, nil, nil + return nil, nil, nil, nil, nil } else { return nil, nil, nil, nil, fmt.Errorf("%s - failed to fetch provider: %w", paymentOrderFields.GatewayID, err) } } - - // Check if provider is private - private orders don't require provision buckets - if orderToken != nil && orderToken.Edges.Provider != nil && orderToken.Edges.Provider.VisibilityMode == providerprofile.VisibilityModePrivate { - isPrivate = true - } + _ = orderToken // Provider validated, continue } - -if provisionBucket == nil && !isPrivate { - err := handleCancellationForIndexedOrder(ctx, network, paymentOrderFields.GatewayID, existingOrder, paymentOrderFields, "Amount is larger than the maximum bucket", refundOrder) - if err != nil { - return nil, nil, nil, nil, nil, fmt.Errorf("failed to handle cancellation: %w", err) - } - return nil, nil, nil, nil, nil, nil -}Note: Since the assignment service's
AssignPaymentOrderWithTriggeralready performs amount-range validation viaamountInRangeForOrder(as shown in the relevant code snippet fromservices/assignment/select.go), the removed bucket check is redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/common/order.go` around lines 1406 - 1458, The code contains leftover provision-bucket logic referencing undefined vars (orderToken, isPrivate, provisionBucket) and returns the wrong number of values; remove the dead conditional that starts with "// Check if provider is private..." and the following "if provisionBucket == nil && !isPrivate" block, delete any associated comments referencing provision buckets, and ensure all return statements in validateAndPreparePaymentOrderData return exactly five values to match the signature (*types.PaymentOrderFields, *ent.Token, *ent.Institution, *ent.FiatCurrency, error) (e.g., return paymentOrderFields, token, institution, currency, nil); keep the provider query result discarded as originally written (or, if you need the provider data later, change the underscore to orderToken at the providerordertoken query site and use it consistently).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@services/common/order.go`:
- Around line 1406-1458: The code contains leftover provision-bucket logic
referencing undefined vars (orderToken, isPrivate, provisionBucket) and returns
the wrong number of values; remove the dead conditional that starts with "//
Check if provider is private..." and the following "if provisionBucket == nil &&
!isPrivate" block, delete any associated comments referencing provision buckets,
and ensure all return statements in validateAndPreparePaymentOrderData return
exactly five values to match the signature (*types.PaymentOrderFields,
*ent.Token, *ent.Institution, *ent.FiatCurrency, error) (e.g., return
paymentOrderFields, token, institution, currency, nil); keep the provider query
result discarded as originally written (or, if you need the provider data later,
change the underscore to orderToken at the providerordertoken query site and use
it consistently).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ab0faa4-fd3e-4c5d-8489-023f526c1b2a
📒 Files selected for processing (4)
controllers/provider/provider_test.gocontrollers/sender/sender_test.goservices/common/order.goutils/test/db.go
💤 Files with no reviewable changes (1)
- controllers/provider/provider_test.go
…nd payin scoring context - GetProviderRate: error on unsupported side or zero effective rate - Clamp ProviderMaxRetryAttempts to min 1 for exclude-list logic - sendOrderRequest: Redis duplicate check before reserve; commit before notify; release fiat + redis cleanup on notify failure; avoid rollback after commit - Fallback: do not fail when fallback_tried_at persistence fails after send - assign_test: restore storage globals after test cleanup - Score history schema: append-only hook, immutable fields, FK edge indexes - ProviderAssignmentRun: indexes on payment_order / provider_order_token edges - FulfillOrder payin: ApplyProviderScoreChange uses reqCtx
- Simplified the return statements in the validateAndPreparePaymentOrderData function to improve clarity and maintainability. - Removed redundant checks and consolidated error handling for provider fetching and cancellation processes.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
services/assignment/select.go (1)
351-371: Consider batching the volume query for better performance.The current implementation queries volume per provider in a loop (N queries for N providers). For a large candidate pool, consider using a single query with
WHERE provider_profile_assigned_orders IN (...)andGROUP BYto reduce database round-trips.♻️ Optional optimization
func recentFiatVolumeByProvider(ctx context.Context, providerIDs []string) (map[string]decimal.Decimal, error) { out := make(map[string]decimal.Decimal) if len(providerIDs) == 0 { return out, nil } since := time.Now().Add(-RecentVolumeWindow) // Build parameterized IN clause args := make([]interface{}, len(providerIDs)+1) placeholders := make([]string, len(providerIDs)) for i, pid := range providerIDs { placeholders[i] = fmt.Sprintf("$%d", i+1) args[i] = pid } args[len(providerIDs)] = since query := fmt.Sprintf(` SELECT provider_profile_assigned_orders, COALESCE(SUM(amount * rate), 0) FROM payment_orders WHERE provider_profile_assigned_orders IN (%s) AND status IN ('validated', 'settled') AND updated_at >= $%d GROUP BY provider_profile_assigned_orders`, strings.Join(placeholders, ","), len(providerIDs)+1) rows, err := storage.DB.QueryContext(ctx, query, args...) if err != nil { return nil, err } defer rows.Close() for rows.Next() { var pid string var sum float64 if err := rows.Scan(&pid, &sum); err != nil { return nil, err } out[pid] = decimal.NewFromFloat(sum) } return out, rows.Err() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/assignment/select.go` around lines 351 - 371, The function recentFiatVolumeByProvider currently runs a DB query per provider; change it to a single parameterized query that selects provider_profile_assigned_orders and COALESCE(SUM(amount * rate),0) FROM payment_orders WHERE provider_profile_assigned_orders IN (...) AND status IN ('validated','settled') AND updated_at >= $N GROUP BY provider_profile_assigned_orders, execute it with storage.DB.QueryContext, iterate rows scanning pid and sum, populate out[pid] = decimal.NewFromFloat(sum) for each row, ensure you defer rows.Close() and return rows.Err(), and keep the early-return when providerIDs is empty; construct the IN clause with positional placeholders and include the since timestamp as the last argument.services/assignment/assign.go (1)
337-414: Note: OTC Redis key creation happens after DB commit.Unlike
sendOrderRequestwhich writes Redis keys before commit (with cleanup on commit failure),assignOtcOrdercommits the DB transaction first (line 337) and then creates the Redis key (lines 389-411). If Redis creation fails after commit, the order is assigned in DB but the Redis key is missing.This asymmetry appears intentional since OTC orders don't involve balance reservation, but consider whether a failed Redis key creation should trigger a DB rollback or reconciliation mechanism.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/assignment/assign.go` around lines 337 - 414, assignOtcOrder currently commits the DB transaction (tx.Commit) before creating the Redis order key (orderKey) so a Redis failure leaves the DB assigned without the Redis marker; move the Redis creation (storage.RedisClient.HSet and ExpireAt for orderKey) to occur before committing the transaction (or immediately after but roll back the DB on Redis error), or implement a compensating action (enqueue a reconciliation job) on Redis failure; specifically, adjust assignOtcOrder to create the Redis hash and TTL using storage.RedisClient.HSet/ExpireAt and only call tx.Commit() after those succeed, or if you must commit first, call tx.Rollback() and return an error when HSet/ExpireAt fail, or alternatively push the failed order ID to a reconciliation queue so a background reconciler (new or existing) can fix missing Redis keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@services/assignment/assign.go`:
- Around line 337-414: assignOtcOrder currently commits the DB transaction
(tx.Commit) before creating the Redis order key (orderKey) so a Redis failure
leaves the DB assigned without the Redis marker; move the Redis creation
(storage.RedisClient.HSet and ExpireAt for orderKey) to occur before committing
the transaction (or immediately after but roll back the DB on Redis error), or
implement a compensating action (enqueue a reconciliation job) on Redis failure;
specifically, adjust assignOtcOrder to create the Redis hash and TTL using
storage.RedisClient.HSet/ExpireAt and only call tx.Commit() after those succeed,
or if you must commit first, call tx.Rollback() and return an error when
HSet/ExpireAt fail, or alternatively push the failed order ID to a
reconciliation queue so a background reconciler (new or existing) can fix
missing Redis keys.
In `@services/assignment/select.go`:
- Around line 351-371: The function recentFiatVolumeByProvider currently runs a
DB query per provider; change it to a single parameterized query that selects
provider_profile_assigned_orders and COALESCE(SUM(amount * rate),0) FROM
payment_orders WHERE provider_profile_assigned_orders IN (...) AND status IN
('validated','settled') AND updated_at >= $N GROUP BY
provider_profile_assigned_orders, execute it with storage.DB.QueryContext,
iterate rows scanning pid and sum, populate out[pid] = decimal.NewFromFloat(sum)
for each row, ensure you defer rows.Close() and return rows.Err(), and keep the
early-return when providerIDs is empty; construct the IN clause with positional
placeholders and include the since timestamp as the last argument.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b3004ff-3445-4065-8d73-86a99c68146a
⛔ Files ignored due to path filters (1)
ent/migrate/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
controllers/provider/provider.goent/client.goent/migrate/migrations/20260324013141_provider_selection_phase1.sqlent/migrate/schema.goent/providerordertokenscorehistory/providerordertokenscorehistory.goent/providerordertokenscorehistory_create.goent/providerordertokenscorehistory_update.goent/runtime/runtime.goent/schema/providerassignmentrun.goent/schema/providerordertokenscorehistory.goservices/assignment/assign.goservices/assignment/assign_test.goservices/assignment/scoring.goservices/assignment/select.goservices/assignment/service.go
✅ Files skipped from review due to trivial changes (3)
- ent/providerordertokenscorehistory/providerordertokenscorehistory.go
- controllers/provider/provider.go
- ent/providerordertokenscorehistory_update.go
🚧 Files skipped from review as they are similar to previous changes (5)
- ent/runtime/runtime.go
- services/assignment/assign_test.go
- services/assignment/scoring.go
- ent/schema/providerassignmentrun.go
- ent/migrate/schema.go
- Removed the creation of ProvisionBucket in sender tests, replacing it with direct Redis bucket min/max values for rate validation. - Updated test cases to reflect changes in bucket handling, ensuring consistency in rate validation logic. - Enhanced comments for clarity regarding the rationale behind the new approach.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
controllers/sender/sender_test.go (1)
215-231: Inconsistent unused variable handling.Line 231 uses
_ = nativeProviderOrderTokento silence the unused variable warning, but the cleaner approach (already used at line 124) is to discard the return value directly at the assignment site.✨ Suggested refactor for consistency
- nativeProviderOrderToken, err := test.AddProviderOrderTokenToProvider(map[string]interface{}{ + _, err = test.AddProviderOrderTokenToProvider(map[string]interface{}{ "provider": providerProfile, "token_id": int(nativeTokenID), "currency_id": currency.ID, "fixed_buy_rate": decimal.NewFromFloat(750.0), "fixed_sell_rate": decimal.NewFromFloat(750.0), "max_order_amount": decimal.NewFromFloat(10000.0), "min_order_amount": decimal.NewFromFloat(1.0), "max_order_amount_otc": decimal.NewFromFloat(10000.0), "min_order_amount_otc": decimal.NewFromFloat(100.0), "settlement_address": "0x1234567890123456789012345678901234567890", "network": testCtx.nativeNetworkIdentifier, }) if err != nil { return fmt.Errorf("AddProviderOrderTokenNative.sender_test: %w", err) } - _ = nativeProviderOrderToken🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/sender/sender_test.go` around lines 215 - 231, The test creates nativeProviderOrderToken but then silences it with `_ = nativeProviderOrderToken`; instead, discard the return value at the call site for consistency with the earlier pattern—replace the binding to nativeProviderOrderToken with a direct discard of the first return (i.e., call test.AddProviderOrderTokenToProvider(...) and assign only the error), keeping the same arguments and error check around the call to AddProviderOrderTokenToProvider to preserve behavior.services/common/order.go (1)
1407-1426: Minor:WithProvider()is unnecessary when discarding the query result.Since the query result is discarded (only checking existence), the
.WithProvider()eager load adds overhead without benefit.💡 Suggested refinement
_, err := db.Client.ProviderOrderToken. Query(). Where( providerordertoken.NetworkEQ(token.Edges.Network.Identifier), providerordertoken.HasProviderWith( providerprofile.IDEQ(paymentOrderFields.ProviderID), providerprofile.HasProviderBalancesWith( providerbalances.HasFiatCurrencyWith(fiatcurrency.CodeEQ(institution.Edges.FiatCurrency.Code)), providerbalances.IsAvailableEQ(true), ), providerprofile.HasUserWith(user.KybVerificationStatusEQ(user.KybVerificationStatusApproved)), ), providerordertoken.HasTokenWith(tokenent.IDEQ(token.ID)), providerordertoken.HasCurrencyWith( fiatcurrency.CodeEQ(institution.Edges.FiatCurrency.Code), ), providerordertoken.SettlementAddressNEQ(""), ). - WithProvider(). Only(ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/common/order.go` around lines 1407 - 1426, The query on db.Client.ProviderOrderToken that builds the token existence check unnecessarily calls WithProvider() which eagerly loads the Provider edge but the result is discarded (Only(ctx) used for existence), so remove the .WithProvider() call from the query chain in the providerordertoken query to avoid extra DB work; update the providerordertoken query (the chain that includes providerordertoken.NetworkEQ(...), providerordertoken.HasProviderWith(...), providerordertoken.HasTokenWith(...), providerordertoken.HasCurrencyWith(...), providerordertoken.SettlementAddressNEQ(...), and ends with Only(ctx)) to omit WithProvider().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@controllers/sender/sender_test.go`:
- Around line 215-231: The test creates nativeProviderOrderToken but then
silences it with `_ = nativeProviderOrderToken`; instead, discard the return
value at the call site for consistency with the earlier pattern—replace the
binding to nativeProviderOrderToken with a direct discard of the first return
(i.e., call test.AddProviderOrderTokenToProvider(...) and assign only the
error), keeping the same arguments and error check around the call to
AddProviderOrderTokenToProvider to preserve behavior.
In `@services/common/order.go`:
- Around line 1407-1426: The query on db.Client.ProviderOrderToken that builds
the token existence check unnecessarily calls WithProvider() which eagerly loads
the Provider edge but the result is discarded (Only(ctx) used for existence), so
remove the .WithProvider() call from the query chain in the providerordertoken
query to avoid extra DB work; update the providerordertoken query (the chain
that includes providerordertoken.NetworkEQ(...),
providerordertoken.HasProviderWith(...), providerordertoken.HasTokenWith(...),
providerordertoken.HasCurrencyWith(...),
providerordertoken.SettlementAddressNEQ(...), and ends with Only(ctx)) to omit
WithProvider().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e42a21f-0dca-445f-b038-99d1ef29da26
📒 Files selected for processing (2)
controllers/sender/sender_test.goservices/common/order.go
Dprof-in-tech
previously approved these changes
Mar 24, 2026
Collaborator
Dprof-in-tech
left a comment
Dprof-in-tech
left a comment
There was a problem hiding this comment.
LGTM!.
But quick question, what's with our failing tests 👀
…nflict resolution - Introduced assignment locking mechanism using Redis to prevent concurrent order assignments. - Added functions to acquire and release locks, along with error handling for lock contention. - Implemented conflict resolution logic to handle cases where an order request is already being processed. - Updated assignment logic to utilize the new locking mechanism, ensuring safe concurrent access. - Enhanced tests to validate the new locking behavior and conflict resolution scenarios.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
services/assignment/select_test.go (1)
224-541: Add regressions for the remaining selector contracts.This suite still doesn't cover a score tie where only
last_order_assigned_atshould break the tie, a skipped attempt still writingProviderAssignmentRun, or the preset-provider stuck-threshold path. Those gaps let the selection/audit bugs inselect.gopass without a failing test.As per coding guidelines, "For new features, add or update tests; use helpers in
utils/test/db.gowhere applicable".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/assignment/select_test.go` around lines 224 - 541, Add three tests in TestAssignment to cover the missing selector regressions: (1) a score-tie case where two providers have identical score but different last_order_assigned_at — create providers with same score via makeAssignProvider/getProviderPOT, set one pot's LastOrderAssignedAt to nil and the other to recent time, call s.AssignPaymentOrder(orderFields(...)) and assert assignedProviderID matches the nil/older pot; (2) a skipped-attempt auditing case — set up an order that no provider can satisfy (e.g. zero balances or out-of-range min/max), invoke s.AssignPaymentOrder and assert the call returns the expected error and that a ProviderAssignmentRun record exists with Result set to AssignmentRunResultNoProvider (query via ac.client.ProviderAssignmentRun where paymentorder.IDEQ(order.ID)); and (3) the preset-provider stuck-threshold path — seed the order_request_<id> Redis key with a preset provider and simulate repeated failed assignments up to the stuck-threshold, then assert chooser behavior and that ProviderAssignmentRun entries are created accordingly; use existing helpers (makeAssignProvider, getProviderPOT, assignedProviderID, orderFields, ac.client) and existing result/trigger constants to locate and implement assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/assignment/select.go`:
- Around line 89-101: When a pre-set provider is skipped due to
utils.GetProviderStuckOrderCount and you push it to Redis via
storage.RedisClient.RPush, also update the in-memory excludeList used by the
candidate-selection loop so the same provider cannot be re-selected in the same
run; locate the check around order.OrderType and
orderConf.ProviderStuckFulfillmentThreshold and after the RPush success append
the provider ID to the local excludeList (or the same in-memory set used by the
candidate loop), and apply the same local-update fix to the other identical
block handling stuck-provider skips.
- Around line 214-239: The sort comparator is incorrectly using recent 24h
volume (from recentFiatVolumeByProvider / volMap) as a tie-breaker before
cmpLastAssigned, which violates the required order (Score DESC, then
last_order_assigned_at ASC NULLS FIRST); update the slices.SortStableFunc
comparator to omit any comparisons using volMap or recentFiatVolumeByProvider
and instead compare b.Score vs a.Score, then call cmpLastAssigned(a,b), then
fall back to ID ordering (a.ID vs b.ID); also remove or stop populating volMap
(recentFiatVolumeByProvider call) since it's no longer used.
- Around line 63-66: The reloaded order (orderEnt returned from
storage.Client.PaymentOrder.Query with WithToken().WithNetwork()) should be
treated as the source of truth: replace any reads from the original/ caller
payload (types.PaymentOrderFields or any incoming order struct) and instead
populate or read Institution, Token, OrderType, Amount, Rate, etc. from orderEnt
(and its related Token/Network) wherever those fields are later used (including
the block around lines 138-158); update the code paths that build or validate
order fields to use orderEnt's getters/relations so stale/partially-hydrated
caller payloads cannot override order behavior.
- Around line 41-60: The early-return branches in AssignPaymentOrder (when
currentOrder.Status != paymentorder.StatusPending, when Redis Exists shows a
duplicate orderKey, and when storage.RedisClient.LRange returns an error) skip
creating the mandatory audit row; before each of those returns you must call
recordAssignmentRun to record the provider_assignment_runs row for this attempt
(include order.ID, an appropriate phase/status/notes indicating
non-pending/duplicate/error, and any error text). Locate the checks around
storage.Client.PaymentOrder.Get, the orderKey/existence check, and the LRange
call, and insert a call to recordAssignmentRun with context and descriptive
fields (use the same error strings already being logged) immediately before
returning so every path creates the audit row.
---
Nitpick comments:
In `@services/assignment/select_test.go`:
- Around line 224-541: Add three tests in TestAssignment to cover the missing
selector regressions: (1) a score-tie case where two providers have identical
score but different last_order_assigned_at — create providers with same score
via makeAssignProvider/getProviderPOT, set one pot's LastOrderAssignedAt to nil
and the other to recent time, call s.AssignPaymentOrder(orderFields(...)) and
assert assignedProviderID matches the nil/older pot; (2) a skipped-attempt
auditing case — set up an order that no provider can satisfy (e.g. zero balances
or out-of-range min/max), invoke s.AssignPaymentOrder and assert the call
returns the expected error and that a ProviderAssignmentRun record exists with
Result set to AssignmentRunResultNoProvider (query via
ac.client.ProviderAssignmentRun where paymentorder.IDEQ(order.ID)); and (3) the
preset-provider stuck-threshold path — seed the order_request_<id> Redis key
with a preset provider and simulate repeated failed assignments up to the
stuck-threshold, then assert chooser behavior and that ProviderAssignmentRun
entries are created accordingly; use existing helpers (makeAssignProvider,
getProviderPOT, assignedProviderID, orderFields, ac.client) and existing
result/trigger constants to locate and implement assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 572e4355-4f05-4601-86ee-25daa30300ca
📒 Files selected for processing (6)
services/assignment/assign.goservices/assignment/constants.goservices/assignment/select.goservices/assignment/select_test.goutils/test/db.goutils/utils.go
✅ Files skipped from review due to trivial changes (1)
- services/assignment/constants.go
🚧 Files skipped from review as they are similar to previous changes (1)
- services/assignment/assign.go
…nd conflict resolution - Implemented a new function to handle concurrent assignment start scenarios, improving error handling for Redis lock contention. - Updated existing methods to accept an parameter, streamlining the assignment process and reducing redundant lock acquisition. - Introduced a timeout constant for audit persistence to ensure it aligns with assignment operation timeouts. - Enhanced the assignment flow to better manage order requests and prevent duplicate assignments in Redis. - Updated relevant tests to reflect changes in the assignment logic and ensure robust handling of concurrent operations.
…ion and stuck order tracking - Introduced a new function, resolveOrderNetwork, to streamline network retrieval for payment orders. - Enhanced stuck order tracking by implementing StuckOrderCountsByProviderIDs, allowing for bulk retrieval of stuck order counts per provider. - Updated assignment logic to utilize the new stuck order count retrieval, improving performance and clarity. - Refactored existing methods to ensure consistency in network handling and error management.
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
2 participants
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.
Description
Implements provider selection phase 1: persistence for scoring
provider_order_tokens, append-only score history (idempotent per payment order + event type), and audit rows for each assignment run.Background
Assignment is being redesigned to use token-level scores, market-rate snapshots on orders, and durable audit logs when providers are selected or no match is found.
What changed
ProviderOrderTokenScoreHistory,ProviderAssignmentRun, using default Ent FK column names (noedge.Field(...)) andON DELETE CASCADEfrompayment_orders/provider_order_tokenswhere defined in schema.ProviderOrderToken:score,last_order_assigned_at; edges to score histories and assignment runs.PaymentOrder: optionalassignment_market_buy_rate/assignment_market_sell_rate; legacy provision bucket column viaStorageKey; edges to the new children with cascade annotations.20260323233838_provider_selection_phase1.sql: new tables and indexes,payment_orders/provision_bucketsadjustments aligned with schema.recordAssignmentRun, reuse of stored market snapshots where applicable.ProviderAssignmentRunwithHasPaymentOrderWith(paymentorder.IDEQ(...))because Ent does not emitPaymentOrderIDEQfor edge-only FKs.Breaking / operational
payment_order_idon these new tables must use the generated FK column names (e.g.payment_order_provider_assignment_runs).Alternatives considered
payment_order_id/Field()on edges; dropped in favor of default Ent naming consistent with other entities.References
closes #714
Testing
go test ./...(or at least./services/assignment/...) with a migrated test database.Developed with: Go as pinned in
go.mod, PostgreSQL.Checklist
mainBy submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
New Features
Improvements
Tests & Migrations