fix(schema): add network field to ProviderOrderToken unique constraint and implement two-phase provider profile updates #516
Conversation
…t and implement two-phase provider profile updates - Update unique index to include network field alongside provider, token, and currency - Fixes bug preventing providers from supporting same token/currency on multiple networks - Resolves issue where provider profile updates failed for new network configurations - Migration included to update existing database constraint - Implement two-phase approach for provider profile updates: * Phase 1: Comprehensive validation of all tokens before database changes * Phase 2: Single transaction execution of all operations * Fixes silent failure bug where profile updates succeeded but token creation failed * Provides detailed validation error reporting for all issues at once * Ensures data consistency with all-or-nothing approach The previous constraint only allowed one ProviderOrderToken per (provider, token, currency) combination, but the application logic queries by (provider, token, currency, network). This mismatch caused 'provider does not support the selected token' errors when trying to configure the same token on different networks. BREAKING CHANGE: Database schema change requires migration to be applied
WalkthroughReworks UpdateProviderProfile into a two‑phase validate‑then‑apply flow; makes ProviderOrderToken.network required and part of the composite unique index; removes ent APIs/predicates for clearing/nil-testing network; updates migrations, generated ent code, controller tests, and test dependencies/mocks; tweaks UpdateSenderProfile error payload shape. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Controller as UpdateProviderProfile
participant Catalog as Token/Currency Service
participant Rates as Rate Engine
participant DB as Database (Tx)
participant Buckets as ProvisionBuckets
Client->>Controller: Submit provider profile update payload
rect rgba(200,220,255,0.25)
note right of Controller: Phase 1 — Validate & collect
loop per token in payload
Controller->>Catalog: validate token & fetch currency
Catalog-->>Controller: token/currency or validation error
Controller->>Rates: compute rate & check slippage
Rates-->>Controller: rate or error
Controller->>DB: fetch existing ProviderOrderToken (if any)
DB-->>Controller: existing token or none
Controller->>Controller: append TokenOperation / validationErrors
end
alt validation errors exist
Controller-->>Client: 400 aggregated validation errors
end
end
rect rgba(220,255,220,0.15)
note right of Controller: Phase 2 — Single transactional apply
Controller->>DB: Begin Tx
loop per token operation
alt update existing
Controller->>DB: update ProviderOrderToken
else create new
Controller->>DB: insert ProviderOrderToken
end
Controller->>Buckets: query matching provision buckets (using converted min/max & rate)
Buckets-->>Controller: bucket list
Controller->>Controller: collect buckets for rebind
end
Controller->>DB: Unbind all provider buckets
Controller->>DB: Rebind aggregated buckets
Controller->>DB: Save provider profile & availability
alt commit ok
DB-->>Controller: Commit
Controller-->>Client: 200 OK
else commit fails
DB-->>Controller: Rollback
Controller-->>Client: 500 error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/accounts/profile.go (1)
231-248: TradingName/HostIdentifier updates are not persisted (set on pre-tx builder, saved on tx builder).You set fields on
update := provider.Update()but later savetxUpdateonly; the earlierupdateis never saved. Move field-sets ontotxUpdateso they are part of the single transaction.Apply near the transaction setup:
- txUpdate := tx.ProviderProfile.Update().Where(providerprofile.IDEQ(provider.ID)) + txUpdate := tx.ProviderProfile.Update().Where(providerprofile.IDEQ(provider.ID)) + // Carry over profile field updates into the transactional builder. + if payload.TradingName != "" { + txUpdate.SetTradingName(payload.TradingName) + } + if payload.HostIdentifier != "" { + // HTTPS validity is already validated above. + txUpdate.SetHostIdentifier(payload.HostIdentifier) + }Optionally remove the earlier non-tx
updatebuilder and itsSet*calls to avoid confusion.Also applies to: 488-493, 581-593
🧹 Nitpick comments (8)
ent/migrate/migrations/20250904102613_add_network_unique_to_provider_order_token.sql (2)
6-6: Pre-check for duplicates to avoid CREATE UNIQUE INDEX failing mid-migration.If duplicates exist on (network, provider, token, currency), index creation will fail. Verify and remediate first.
Run this SQL in the target DB to audit:
SELECT provider_profile_order_tokens, token_provider_order_tokens, fiat_currency_provider_order_tokens, network, COUNT(*) AS dup_count FROM provider_order_tokens GROUP BY 1,2,3,4 HAVING COUNT(*) > 1;If you expect high write load during migration, consider a low-lock alternative:
-- 1) Create concurrently (requires running outside a transaction): CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS providerordertoken_network_pro_78d86f7d16ed79216b911727a796323a ON provider_order_tokens (network, provider_profile_order_tokens, token_provider_order_tokens, fiat_currency_provider_order_tokens); -- 2) Optionally attach as a constraint (keeps semantics explicit): -- ALTER TABLE provider_order_tokens -- ADD CONSTRAINT providerordertoken_network_provider_token_currency_uniq -- UNIQUE USING INDEX providerordertoken_network_pro_78d86f7d16ed79216b911727a796323a; -- 3) Drop old index afterwards (CONCURRENTLY) if your migration runner supports it.
1-2: Make index drop idempotent.Use IF EXISTS to avoid failures across environments where the old index name may differ or be missing.
-DROP INDEX "providerordertoken_provider_pr_6a0d0c64fd46fb967691502cb58fa192"; +DROP INDEX IF EXISTS "providerordertoken_provider_pr_6a0d0c64fd46fb967691502cb58fa192";ent/migrate/schema.go (1)
489-492: Confirm column order intent for the unique index (network-leading).Index leading column is network. If typical lookups filter by provider/token/currency first, consider reordering to match access paths; otherwise keep as-is if network is always in predicates.
ent/schema/providerordertoken.go (2)
39-40: Harden input: enforce non-empty and canonical format for network.Prevent empty strings and accidental casing/whitespace drift.
-import ( - "entgo.io/ent" - "entgo.io/ent/schema/edge" - "entgo.io/ent/schema/field" - "entgo.io/ent/schema/index" - "github.com/shopspring/decimal" -) +import ( + "entgo.io/ent" + "entgo.io/ent/schema/edge" + "entgo.io/ent/schema/field" + "entgo.io/ent/schema/index" + "github.com/shopspring/decimal" + "fmt" + "regexp" + "strings" +) @@ - field.String("address").Optional(), - field.String("network"), + field.String("address").Optional(), + field.String("network"). + NotEmpty(). + Validate(func(v string) error { + // enforce lowercase kebab/underscore alphanumerics, e.g., "ethereum", "polygon", "bsc_testnet" + s := strings.TrimSpace(v) + if s != v || s == "" { + return fmt.Errorf("network must be non-empty and trimmed") + } + ok, _ := regexp.MatchString(`^[a-z0-9_-]+$`, s) + if !ok || s != strings.ToLower(s) { + return fmt.Errorf("network must be lowercase [a-z0-9_-]") + } + return nil + }),
64-66: Make index definition reflect intended column order explicitly.Ent generates network as the leading column; make that intention explicit for readers and future diffs.
- // Define a unique index across multiple fields including network. - index.Edges("provider", "token", "currency").Fields("network").Unique(), + // Unique on (network, provider, token, currency). + index.Fields("network").Edges("provider", "token", "currency").Unique(),controllers/accounts/profile.go (3)
576-579: Deduplicate buckets before adding to avoid duplicate M2M rows.If multiple tokens map to the same bucket,
AddProvisionBuckets(allBuckets...)may attempt duplicate links.Example:
uniq := make(map[int]*ent.ProvisionBucket, len(allBuckets)) for _, b := range allBuckets { uniq[b.ID] = b } dedup := make([]*ent.ProvisionBucket, 0, len(uniq)) for _, b := range uniq { dedup = append(dedup, b) } txUpdate.ClearProvisionBuckets() txUpdate.AddProvisionBuckets(dedup...)
596-603: Rollback after failed Commit is redundant.After
Commit()returns, the tx is closed; callingRollback()typically returns an error and adds noise to logs.Apply:
- if err := tx.Commit(); err != nil { - if rollbackErr := tx.Rollback(); rollbackErr != nil { - logger.Errorf("Failed to rollback transaction: %v", rollbackErr) - } - logger.Errorf("Failed to commit transaction: %v", err) - u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to update profile", nil) - return - } + if err := tx.Commit(); err != nil { + logger.Errorf("Failed to commit transaction: %v", err) + u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to update profile", nil) + return + }
479-485: Race window between validation and create.Between Phase 1 and inserts, another request could create the same (provider, token, currency, network), causing a unique-constraint error. Today this returns a 500.
Consider one of:
- Use
OnConflict(sql.DoNothing())and then fetch-or-update, returning a 409/422 with a descriptive message.- Re-check existence within the tx before create, or switch to an upsert on the composite key.
If you want, I can draft the upsert-based path.
Also applies to: 520-533
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
ent/migrate/migrations/atlas.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
controllers/accounts/profile.go(7 hunks)ent/migrate/migrations/20250904102613_add_network_unique_to_provider_order_token.sql(1 hunks)ent/migrate/schema.go(2 hunks)ent/mutation.go(0 hunks)ent/providerordertoken/where.go(0 hunks)ent/providerordertoken_create.go(1 hunks)ent/providerordertoken_update.go(0 hunks)ent/schema/providerordertoken.go(2 hunks)
💤 Files with no reviewable changes (3)
- ent/providerordertoken/where.go
- ent/providerordertoken_update.go
- ent/mutation.go
🧰 Additional context used
🧬 Code graph analysis (3)
ent/providerordertoken_create.go (3)
ent/providerordertoken/where.go (1)
Network(100-102)ent/network.go (2)
Network(18-48)Network(82-101)ent/ent.go (1)
ValidationError(222-225)
ent/migrate/schema.go (1)
ent/providerordertoken/providerordertoken.go (1)
Columns(70-82)
controllers/accounts/profile.go (8)
types/types.go (2)
ProviderOrderTokenPayload(252-262)ErrorData(565-568)ent/schema/providerordertoken.go (5)
ProviderOrderToken(12-14)ProviderOrderToken(17-21)ProviderOrderToken(24-41)ProviderOrderToken(44-59)ProviderOrderToken(62-67)utils/http.go (1)
APIResponse(23-29)ent/providerordertoken/providerordertoken.go (2)
ConversionRateType(117-117)ConversionRateTypeFloating(122-122)utils/utils.go (1)
AbsPercentageDeviation(202-209)ent/ent.go (2)
IsNotFound(257-263)Query(48-48)ent/tx.go (1)
Tx(13-74)ent/provisionbucket/where.go (2)
MinAmountLTE(110-112)MaxAmountGTE(140-142)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (2)
ent/migrate/schema.go (1)
457-457: LGTM: network now non-nullable matches the migration intent.ent/providerordertoken_create.go (1)
356-377: No ProviderOrderToken OnConflict usage found—no changes needed. I searched the entire codebase for anyOnConflictorOnConflictColumnscalls onProviderOrderTokenand found none.
ent/migrate/migrations/20250904102613_add_network_unique_to_provider_order_token.sql
Show resolved
Hide resolved
- Use UpdateOneID for transaction-bound token updates - Add backfill logic to migration before enforcing NOT NULL constraint - Fix bucket selection logic to use proper range intersection - Ensure providers are assigned to all buckets that overlap their range - Prevents migration failure on existing NULL network values - More efficient and safer database operations
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
controllers/accounts/profile.go (3)
231-248: TradingName/HostIdentifier are never persisted and not included in the transaction.You build
update := provider.Update()and set fields, but never callupdate.Save. Also, these changes should be part of the Phase 2 transaction. Apply them totxUpdateinstead.Minimal fix:
- update := provider.Update() - - if payload.TradingName != "" { - update.SetTradingName(payload.TradingName) - } - - if payload.HostIdentifier != "" { - // Validate HTTPS protocol - if !u.IsValidHttpsUrl(payload.HostIdentifier) { - ... - } - update.SetHostIdentifier(payload.HostIdentifier) - } + // Defer persistence to txUpdate below; keep HTTPS validation here. @@ - txUpdate := tx.ProviderProfile.Update().Where(providerprofile.IDEQ(provider.ID)) + txUpdate := tx.ProviderProfile.Update().Where(providerprofile.IDEQ(provider.ID)) + if payload.TradingName != "" { + txUpdate.SetTradingName(payload.TradingName) + } + if payload.HostIdentifier != "" { + txUpdate.SetHostIdentifier(payload.HostIdentifier) + } @@ - _, err = txUpdate.Save(ctx) + _, err = txUpdate.Save(ctx)Also applies to: 487-490, 584-586
250-321: Currency availability updates run outside the transaction; move into Phase 2.The ProviderCurrencies create/update uses
storage.Clientbefore the transaction, breaking the all-or-nothing guarantee described in the PR. Re-run that logic usingtx.ProviderCurrencieswithin Phase 2.Approach:
- Remove Lines 250-321.
- After
tx := storage.Client.Tx(ctx)(Lines 479-486), re-implement the same create/update usingtx.FiatCurrency,tx.ProviderCurrencies, and include it beforetxUpdate.Save(ctx). Keep the same validation and logging.Also applies to: 479-486
341-352: Check Min/Max relationship and positive effective rate.Add validation to ensure
MinOrderAmount <= MaxOrderAmountand computedrate > 0to avoid nonsensical ranges and negative conversions.for _, tokenPayload := range payload.Tokens { + if tokenPayload.MinOrderAmount.GreaterThan(tokenPayload.MaxOrderAmount) { + validationErrors = append(validationErrors, types.ErrorData{ + Field: "Tokens", + Message: fmt.Sprintf("MinOrderAmount must be <= MaxOrderAmount for %s", tokenPayload.Symbol), + }) + continue + } @@ if tokenPayload.ConversionRateType == providerordertoken.ConversionRateTypeFixed { rate = tokenPayload.FixedConversionRate } else { rate = currency.MarketRate.Add(tokenPayload.FloatingConversionRate) } + if !rate.GreaterThan(decimal.Zero) { + validationErrors = append(validationErrors, types.ErrorData{ + Field: "Tokens", + Message: fmt.Sprintf("Computed rate must be > 0 for %s", tokenPayload.Symbol), + }) + continue + }Also applies to: 435-446
♻️ Duplicate comments (2)
controllers/accounts/profile.go (2)
549-576: Bucket selection OK; dedupe and always clear before re-attach.
- Good fix replacing OR with an interval overlap.
- To avoid duplicate edges and stale assignments, dedupe
allBucketsand clear regardless of length.- allBuckets = append(allBuckets, buckets...) + // dedupe by bucket ID + existing := make(map[interface{}]struct{}) + for _, b := range buckets { + id := b.ID + if _, ok := existing[id]; !ok { + existing[id] = struct{}{} + allBuckets = append(allBuckets, b) + } + } @@ - if len(allBuckets) > 0 { - txUpdate.ClearProvisionBuckets() - txUpdate.AddProvisionBuckets(allBuckets...) - } + txUpdate.ClearProvisionBuckets() + if len(allBuckets) > 0 { + txUpdate.AddProvisionBuckets(allBuckets...) + }Also applies to: 578-582
1-1: Binding/JSON tag mismatches still block intended behavior.Cross-file issues to address (unchanged here but required for this flow to work):
- types/types.go:
RateSlippagetag should allow omission; update toomitempty,gte=0.1.- types/types.go:
Networktag appears malformed (:"network"). It must bejson:"network"or requests won’t bind.Proposed fixes (types/types.go):
- RateSlippage decimal.Decimal `json:"rateSlippage" binding:"gte=0.1"` + RateSlippage decimal.Decimal `json:"rateSlippage" binding:"omitempty,gte=0.1"` - Network string :"network" binding:"required"` + Network string `json:"network" binding:"required"`Run to verify:
#!/bin/bash rg -n 'ProviderOrderTokenPayload' -nP rg -nP 'RateSlippage.*binding:' --types go -C2 rg -nP 'json"\\s*:\\s*"network' --types go -C2
🧹 Nitpick comments (6)
ent/migrate/migrations/20250904102613_add_network_unique_to_provider_order_token.sql (3)
2-2: Make index drop resilient across environments.Use IF EXISTS to avoid failure where the old index name is missing (e.g., fresh envs).
-DROP INDEX "providerordertoken_provider_pr_6a0d0c64fd46fb967691502cb58fa192"; +DROP INDEX IF EXISTS "providerordertoken_provider_pr_6a0d0c64fd46fb967691502cb58fa192";
13-15: Preflight for dupes that would violate the new unique constraint.If historical data has duplicates for (network, provider_profile_order_tokens, token_provider_order_tokens, fiat_currency_provider_order_tokens), the CREATE UNIQUE INDEX will fail. Add a pre-check (and optionally a cleanup or a fail-fast) before creating the index.
Example sanity check (safe for dev/staging):
-- Detect duplicates that would violate the new unique index SELECT network, provider_profile_order_tokens, token_provider_order_tokens, fiat_currency_provider_order_tokens, COUNT(*) FROM provider_order_tokens GROUP BY 1,2,3,4 HAVING COUNT(*) > 1;If this returns rows, decide whether to consolidate or block the migration.
4-11: Handle tokens without a linked network (defensive).If any token lacks a
networkslinkage, those rows remain NULL and the subsequentSET NOT NULLwill fail. Consider a LEFT JOIN + separate check, or keep your INNER JOIN and add a follow-up guard that raises in non-prod or logs counts.-- Optional: fail-fast if any NULL remain (dev/staging) -- SELECT COUNT(*) AS remaining_nulls FROM provider_order_tokens WHERE network IS NULL;controllers/accounts/profile.go (3)
418-433: Avoid float constructors for decimal thresholds; use exact constants.Use
decimal.RequireFromStringto prevent float rounding surprises. Also consider moving thresholds to config.- if tokenPayload.RateSlippage.IsZero() { - tokenPayload.RateSlippage = decimal.NewFromFloat(0) - } else if tokenPayload.RateSlippage.LessThan(decimal.NewFromFloat(0.1)) { + if tokenPayload.RateSlippage.IsZero() { + tokenPayload.RateSlippage = decimal.Zero + } else if tokenPayload.RateSlippage.LessThan(decimal.RequireFromString("0.1")) { ... - } else if rate.Mul(tokenPayload.RateSlippage.Div(decimal.NewFromFloat(100))).GreaterThan(currency.MarketRate.Mul(decimal.NewFromFloat(0.05))) { + } else if rate.Mul(tokenPayload.RateSlippage.Div(decimal.RequireFromString("100"))). + GreaterThan(currency.MarketRate.Mul(decimal.RequireFromString("0.05"))) {
496-506: Surface unique-constraint violations as validation errors (not 500).Races between Phase 1 and Phase 2 can still trigger unique index errors. Convert
ent.IsConstraintError(err)into a 400 with a token-scoped error message.- if err != nil { + if err != nil { + if ent.IsConstraintError(err) { + _ = tx.Rollback() + u.APIResponse(ctx, http.StatusBadRequest, "error", "Validation failed", + []types.ErrorData{{ Field: "Tokens", Message: fmt.Sprintf("Duplicate configuration for %s on %s", op.TokenPayload.Symbol, op.TokenPayload.Network) }}, + ) + return + } if rollbackErr := tx.Rollback(); rollbackErr != nil { ... } ... }Apply similarly to the create path.
Also applies to: 521-545
599-606: Don’t rollback after a failed Commit.After
Commit()returns an error, the transaction is no longer usable. Drop the extra rollback attempt.- if err := tx.Commit(); err != nil { - if rollbackErr := tx.Rollback(); rollbackErr != nil { - logger.Errorf("Failed to rollback transaction: %v", rollbackErr) - } - logger.Errorf("Failed to commit transaction: %v", err) - u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to update profile", nil) - return - } + if err := tx.Commit(); err != nil { + logger.Errorf("Failed to commit transaction: %v", err) + u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to update profile", nil) + return + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
controllers/accounts/profile.go(7 hunks)ent/migrate/migrations/20250904102613_add_network_unique_to_provider_order_token.sql(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/accounts/profile.go (8)
types/types.go (2)
ProviderOrderTokenPayload(252-262)ErrorData(565-568)ent/fiatcurrency.go (2)
FiatCurrency(18-44)FiatCurrency(98-119)ent/schema/providerordertoken.go (5)
ProviderOrderToken(12-14)ProviderOrderToken(17-21)ProviderOrderToken(24-41)ProviderOrderToken(44-59)ProviderOrderToken(62-67)utils/http.go (1)
APIResponse(23-29)utils/utils.go (1)
AbsPercentageDeviation(202-209)ent/tx.go (1)
Tx(13-74)ent/providerprofile.go (2)
ProviderProfile(20-43)ProviderProfile(136-153)ent/provisionbucket/where.go (3)
ID(15-17)MinAmountLTE(110-112)MaxAmountGTE(140-142)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (1)
ent/migrate/migrations/20250904102613_add_network_unique_to_provider_order_token.sql (1)
4-11: Backfill-before-NOT-NULL looks correct.Good: you backfill
networkfromtokens -> networksbeforeSET NOT NULL, preventing immediate migration failure.
…update logic - Modify validation messages to include context for TST in error responses. - Refactor provider profile update logic to ensure basic fields are set within the transaction. - Enhance error handling for validation failures to provide clearer feedback on issues encountered. - Introduce a more structured approach to setting profile fields during updates.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
controllers/accounts/profile.go (5)
72-77: Rollback not guaranteed on early returns — add a deferred rollbackMultiple early returns inside the transaction path can leave the transaction open and leak the connection. Ensure rollback on any non-commit path.
tx, err := storage.Client.Tx(ctx) if err != nil { u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to update profile", nil) return } +defer func() { _ = tx.Rollback() }()
84-86: Avoid unintended clearing of DomainWhitelist when omittedCurrent condition sets DomainWhitelist to nil if the payload omits it and the existing value is non-nil. Update only when provided.
-if payload.DomainWhitelist != nil || (payload.DomainWhitelist == nil && sender.DomainWhitelist != nil) { - update.SetDomainWhitelist(payload.DomainWhitelist) -} +if payload.DomainWhitelist != nil { + update.SetDomainWhitelist(payload.DomainWhitelist) +}
113-117: Unify error response shape to []types.ErrorDataOther validation errors return a slice; these two paths return a single struct, causing inconsistent API shapes.
- u.APIResponse(ctx, http.StatusBadRequest, "error", "Failed to validate payload", types.ErrorData{ + u.APIResponse(ctx, http.StatusBadRequest, "error", "Failed to validate payload", []types.ErrorData{{ Field: "FeeAddress", Message: "Invalid Tron address", - }) + }})- u.APIResponse(ctx, http.StatusBadRequest, "error", "Failed to validate payload", types.ErrorData{ + u.APIResponse(ctx, http.StatusBadRequest, "error", "Failed to validate payload", []types.ErrorData{{ Field: "FeeAddress", Message: "Invalid Ethereum address", - }) + }})Also applies to: 121-127
244-316: Currency availability updates run outside the all-or-nothing transactionThis breaks atomicity: availability can change even if token validations fail later. Move this block into Phase 2 using the same tx.
Minimal approach: capture intent, then apply inside Phase 2 before committing.
// Before tx: compute desired availability update intent (no writes) var availabilityOp *struct{ currencyCode string; isAvailable bool } if payload.Currency != "" { availabilityOp = &struct{ currencyCode string; isAvailable bool }{payload.Currency, payload.IsAvailable} }Inside Phase 2, just after tx start and before token ops:
if availabilityOp != nil { curr, err := tx.FiatCurrency.Query(). Where(fiatcurrency.CodeEQ(availabilityOp.currencyCode), fiatcurrency.IsEnabled(true)). Only(ctx) if err != nil { u.APIResponse(ctx, http.StatusBadRequest, "error", "Currency not supported", nil); return } pc, err := tx.ProviderCurrencies.Query(). Where(providercurrencies.HasProviderWith(providerprofile.IDEQ(provider.ID)), providercurrencies.HasCurrencyWith(fiatcurrency.CodeEQ(availabilityOp.currencyCode))). Only(ctx) if ent.IsNotFound(err) { _, err = tx.ProviderCurrencies.Create().SetProviderID(provider.ID).SetCurrency(curr). SetAvailableBalance(decimal.Zero).SetTotalBalance(decimal.Zero).SetReservedBalance(decimal.Zero). SetIsAvailable(availabilityOp.isAvailable).Save(ctx) } else if err == nil { _, err = pc.Update().SetIsAvailable(availabilityOp.isAvailable).Save(ctx) } if err != nil { u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to update availability", nil); return } }
667-678: Panic risk: tokensPayload length computed from wrong sourceYou size the slice from sender.Edges.OrderTokens, but iterate over senderToken (query result). If edges aren’t preloaded, length is zero and this panics. Use len(senderToken).
- tokensPayload := make([]types.SenderOrderTokenResponse, len(sender.Edges.OrderTokens)) - for i, token := range senderToken { + tokensPayload := make([]types.SenderOrderTokenResponse, len(senderToken)) + for i, token := range senderToken {
♻️ Duplicate comments (1)
controllers/accounts/profile.go (1)
409-423: Binding tag vs controller logic mismatch for RateSlippage (repeat)Controller allows omitted/zero slippage and enforces lower bound in code, but the binding tag in types requires gte=0.1, which would reject zero before reaching here. Update the binding to allow omission.
Proposed change in types/types.go:
- RateSlippage decimal.Decimal `json:"rateSlippage" binding:"gte=0.1"` + RateSlippage decimal.Decimal `json:"rateSlippage" binding:"omitempty,gte=0.1"`Script to verify:
#!/bin/bash rg -nP 'RateSlippage\s+decimal\.Decimal.*binding:"[^"]*"' --type=go
🧹 Nitpick comments (9)
controllers/provider/provider_test.go (3)
717-719: Assert the new NodeInfo contract (serviceInfo.currencies) to prevent silent regressions.You've updated the mock to nest currencies under data.serviceInfo.currencies but the test doesn't verify this shape. Add assertions to lock the contract.
Insert after Line 746:
data, ok := response.Data.(map[string]interface{}) assert.True(t, ok, "response.Data should be a map") svc, ok := data["serviceInfo"].(map[string]interface{}) assert.True(t, ok, "data.serviceInfo should be a map") curr, ok := svc["currencies"].([]interface{}) assert.True(t, ok, "data.serviceInfo.currencies should be an array") assert.Contains(t, curr, "NGN")
1514-1528: Use precise decimal constructors for min/max and add a quick linkage check.Avoid float -> decimal conversions in tests; prefer string constructors to eliminate rounding risk. Also, a small assertion that the order is linked to the newly created provision bucket will make this scenario more robust.
Apply:
- provisionBucket, err := db.Client.ProvisionBucket. + provisionBucket, err := db.Client.ProvisionBucket. Create(). - SetMinAmount(decimal.NewFromFloat(100.0)). - SetMaxAmount(decimal.NewFromFloat(1000.0)). + SetMinAmount(decimal.RequireFromString("100")). + SetMaxAmount(decimal.RequireFromString("1000")). SetCurrency(testCtx.currency). Save(context.Background())Then (outside this hunk), immediately after saving the updated order:
pb, err := order.QueryProvisionBucket().Only(context.Background()) assert.NoError(t, err) assert.Equal(t, provisionBucket.ID, pb.ID)
1536-1541: Remove unused provider profile creation to reduce test overhead.The created provider profile is not used anymore in this test block. Dropping it will speed up the test and reduce DB noise.
- _, err = test.CreateTestProviderProfile(map[string]interface{}{ - "user_id": user.ID, - "currency_id": testCtx.currency.ID, - }) - assert.NoError(t, err)controllers/sender/sender_test.go (1)
234-247: Prefer DeactivateAndReset and assert the mock was exercisedUse httpmock.DeactivateAndReset() to avoid responder leakage across tests and add an assertion that the registered responder was actually called.
- httpmock.Activate() - defer httpmock.Deactivate() + httpmock.Activate() + defer httpmock.DeactivateAndReset()Optionally verify the call occurred (after the request):
info := httpmock.GetCallCountInfo() assert.Equal(t, 1, info["POST https://engine.thirdweb.com/v1/accounts"])services/etherscan_test.go (1)
13-24: Use t.Setenv for deterministic, race-free env handlingt.Setenv automatically restores the prior state and is concurrency-safe for tests.
- // Set a test API key for this test - originalKey := os.Getenv("ETHERSCAN_API_KEY") - defer func() { - if originalKey != "" - { os.Setenv("ETHERSCAN_API_KEY", originalKey) } - else - { os.Unsetenv("ETHERSCAN_API_KEY") } - }() - - os.Setenv("ETHERSCAN_API_KEY", "test-api-key") + t.Setenv("ETHERSCAN_API_KEY", "test-api-key")controllers/accounts/profile.go (4)
589-592: Deduplicate buckets before assignment to avoid duplicate join rowsCollecting across tokens can repeat buckets; deduplicate by ID before AddProvisionBuckets.
- if len(allBuckets) > 0 { - txUpdate.ClearProvisionBuckets() - txUpdate.AddProvisionBuckets(allBuckets...) - } + if len(allBuckets) > 0 { + seen := make(map[int]struct{}, len(allBuckets)) + var uniq []*ent.ProvisionBucket + for _, b := range allBuckets { + if _, ok := seen[b.ID]; ok { continue } + seen[b.ID] = struct{}{} + uniq = append(uniq, b) + } + txUpdate.ClearProvisionBuckets() + txUpdate.AddProvisionBuckets(uniq...) + }
608-616: Simplify commit/rollback handlingAttempting Rollback after a failed Commit usually returns “already completed” and adds noise. Use a single deferred rollback guard and commit once.
- if err := tx.Commit(); err != nil { - if rollbackErr := tx.Rollback(); rollbackErr != nil { - logger.Errorf("Failed to rollback transaction: %v", rollbackErr) - } - logger.Errorf("Failed to commit transaction: %v", err) - u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to update profile", nil) - return - } + if err := tx.Commit(); err != nil { + logger.Errorf("Failed to commit transaction: %v", err) + u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to update profile", nil) + return + }Note: keep the earlier deferred rollback to cover all early-return paths.
491-497: Validate VisibilityMode against allowed values (defensive)If binding doesn’t already restrict values, invalid strings will pass through here. Consider enforcing oneof at binding or validating before SetVisibilityMode.
If needed in payload struct:
- VisibilityMode string `json:"visibilityMode"` + VisibilityMode string `json:"visibilityMode" binding:"omitempty,oneof=private public"`
417-423: Prefer exact-decimal constants to avoid float roundingUse decimal.RequireFromString for 0.1 and 0.05 to avoid float parsing artifacts.
- } else if tokenPayload.RateSlippage.LessThan(decimal.NewFromFloat(0.1)) { + } else if tokenPayload.RateSlippage.LessThan(decimal.RequireFromString("0.1")) { - } else if rate.Mul(tokenPayload.RateSlippage.Div(decimal.NewFromFloat(100))).GreaterThan(currency.MarketRate.Mul(decimal.NewFromFloat(0.05))) { + } else if rate.Mul(tokenPayload.RateSlippage.Div(decimal.RequireFromString("100"))).GreaterThan(currency.MarketRate.Mul(decimal.RequireFromString("0.05"))) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
controllers/accounts/profile.go(8 hunks)controllers/accounts/profile_test.go(2 hunks)controllers/provider/provider_test.go(3 hunks)controllers/sender/sender_test.go(2 hunks)services/etherscan_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
controllers/provider/provider_test.go (2)
ent/client.go (1)
Client(48-104)utils/test/db.go (2)
CreateTestUser(33-62)CreateTestProviderProfile(452-502)
controllers/accounts/profile.go (9)
types/types.go (2)
ProviderOrderTokenPayload(252-262)ErrorData(565-568)ent/schema/providerordertoken.go (5)
ProviderOrderToken(12-14)ProviderOrderToken(17-21)ProviderOrderToken(24-41)ProviderOrderToken(44-59)ProviderOrderToken(62-67)utils/http.go (1)
APIResponse(23-29)ent/providerordertoken/providerordertoken.go (2)
ConversionRateType(117-117)ConversionRateTypeFloating(122-122)utils/utils.go (1)
AbsPercentageDeviation(202-209)ent/client.go (1)
Client(48-104)ent/tx.go (1)
Tx(13-74)ent/schema/providerprofile.go (3)
ProviderProfile(15-17)ProviderProfile(20-42)ProviderProfile(45-66)ent/provisionbucket/where.go (2)
MinAmountLTE(110-112)MaxAmountGTE(140-142)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (3)
controllers/provider/provider_test.go (1)
1545-1545: LGTM: Using the order’s providerId explicitly clarifies the exclusion scenario.This makes the intent clear and aligns the test data with the scenario under test.
controllers/accounts/profile_test.go (2)
466-467: LGTM: token-aware error messageExpectation matches the controller’s new token-specific validation message.
486-487: LGTM: token-aware error messageExpectation matches the controller’s new token-specific validation message.
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 (3)
controllers/sender/sender_test.go (3)
135-137: Remove random sleeps; they make tests slow and flaky.
This loop can add minutes to CI and doesn’t add determinism for ordering.Apply this diff:
- for i := 0; i < 9; i++ { - time.Sleep(time.Duration(float64(rand.Intn(12))) * time.Second) + for i := 0; i < 9; i++ {If you need timestamp variance for the “ordering” test, rely on DB precision or set CreatedAt explicitly (if the ent schema exposes SetCreatedAt).
203-210: Don’t depend on a local Redis; use miniredis for hermetic tests.
Connecting to localhost:6379 will fail in CI environments without Redis.Apply this diff:
- // Set up mock Redis client - redisClient := redis.NewClient(&redis.Options{ - Addr: "localhost:6379", - }) - defer redisClient.Close() - - db.RedisClient = redisClient + // Set up in-memory Redis + mr, err := miniredis.Run() + assert.NoError(t, err) + defer mr.Close() + + db.RedisClient = redis.NewClient(&redis.Options{Addr: mr.Addr()})Add this import:
import "github.com/alicebob/miniredis/v2"
332-350: Auth header missing and timestamp is nil in query; request may be rejected.
This subtest uses only API-Key and formats a nil timestamp. Align with other GETs by sending HMAC over a real timestamp.Apply this diff:
- headers := map[string]string{ - "API-Key": testCtx.apiKey.ID.String(), - } - - res, err = test.PerformRequest(t, "GET", fmt.Sprintf("/sender/orders/%s?timestamp=%v", paymentOrderUUID.String(), payload["timestamp"]), nil, headers, router) + ts := time.Now().Unix() + sigPayload := map[string]interface{}{"timestamp": ts} + sig := token.GenerateHMACSignature(sigPayload, testCtx.apiKeySecret) + headers := map[string]string{ + "Authorization": "HMAC " + testCtx.apiKey.ID.String() + ":" + sig, + } + + res, err = test.PerformRequest(t, "GET", fmt.Sprintf("/sender/orders/%s?timestamp=%v", paymentOrderUUID.String(), ts), nil, headers, router)
♻️ Duplicate comments (1)
controllers/sender/sender_test.go (1)
234-237: httpmock.Activate() won’t intercept non-default HTTP clients; use ActivateNonDefault and reset properly.
The engine client constructs its own http.Client/Transport, so these responders won’t fire. Also prefer DeactivateAndReset to avoid leakage across subtests.Apply this diff (wire in the actual engine client used in production code):
- // Activate httpmock - httpmock.Activate() - defer httpmock.Deactivate() + // Activate httpmock for the actual engine HTTP client + // TODO: replace `engineHTTPClient` with the real *http.Client used by engine/webhook calls. + httpmock.ActivateNonDefault(engineHTTPClient) + defer httpmock.DeactivateAndReset()If the engine client isn’t accessible, inject it (constructor param or setter) so tests can pass its *http.Client here.
🧹 Nitpick comments (6)
controllers/provider/provider_test.go (4)
717-719: Add assertions for the new nested NodeInfo shape (serviceInfo.currencies).You updated the mock to the new structure but the test doesn’t assert it, so regressions could slip by unnoticed.
Consider asserting the nested fields after unmarshalling:
data, ok := response.Data.(map[string]interface{}) assert.True(t, ok) svc, ok := data["serviceInfo"].(map[string]interface{}) assert.True(t, ok, "data.serviceInfo missing") curr, ok := svc["currencies"].([]interface{}) assert.True(t, ok, "data.serviceInfo.currencies missing") assert.Contains(t, curr, "NGN")
1515-1521: Prefer exact decimal constructors in tests to avoid float-to-decimal rounding noise.- SetMinAmount(decimal.NewFromFloat(100.0)). - SetMaxAmount(decimal.NewFromFloat(1000.0)). + SetMinAmount(decimal.NewFromInt(100)). + SetMaxAmount(decimal.NewFromInt(1000)).
1536-1539: Fix unused provider profile and align the scenario name/intent.You create another provider but discard it, then set providerId to the same provider as the order. If the scenario is “exclude Order For Provider” (i.e., order mapped to a different provider), use the created provider’s ID. Otherwise, rename the test. Pick one and proceed.
- _, err = test.CreateTestProviderProfile(map[string]interface{}{ + otherProvider, err := test.CreateTestProviderProfile(map[string]interface{}{ "user_id": user.ID, "currency_id": testCtx.currency.ID, }) assert.NoError(t, err) @@ - "providerId": testCtx.provider.ID, // Use the same provider as the order + "providerId": otherProvider.ID, // Map to a different provider to exercise exclusionAlso applies to: 1546-1546
1583-1596: DRY the provisioning-bucket setup and use exact decimals here too.Same bucket setup appears twice; a tiny helper reduces duplication. Also swap float -> int constructors to keep precision stable.
- SetMinAmount(decimal.NewFromFloat(100.0)). - SetMaxAmount(decimal.NewFromFloat(1000.0)). + SetMinAmount(decimal.NewFromInt(100)). + SetMaxAmount(decimal.NewFromInt(1000)).Example helper you can drop into this test file to replace both blocks:
func createAndAttachProvisionBucket(t *testing.T, order *ent.LockPaymentOrder, cur *ent.FiatCurrency, min, max int64) *ent.LockPaymentOrder { t.Helper() bkt, err := db.Client.ProvisionBucket. Create(). SetMinAmount(decimal.NewFromInt(min)). SetMaxAmount(decimal.NewFromInt(max)). SetCurrency(cur). Save(context.Background()) assert.NoError(t, err) updated, err := order.Update().SetProvisionBucket(bkt).Save(context.Background()) assert.NoError(t, err) return updated }controllers/sender/sender_test.go (2)
516-523: Fail fast on timestamp parse errors.
Return without an assertion hides failures; assert instead.Apply this diff:
- if err != nil { - return - } + assert.NoError(t, err) @@ - if err != nil { - return - } + assert.NoError(t, err)
804-804: Fix misleading comment about expected fee magnitude.
Current math yields ~0.000007 USD, not ~0.005.Apply this diff:
- // Assert the totalFeeEarnings value (5% of 100 NGN / 950 market rate ≈ 0.005 USD) + // Assert the totalFeeEarnings value (~5% of 100 NGN, converted via 750 and 950 rates ≈ 0.000007 USD)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
controllers/provider/provider_test.go(4 hunks)controllers/sender/sender_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/provider/provider_test.go (2)
ent/client.go (1)
Client(48-104)utils/test/db.go (2)
CreateTestUser(33-62)CreateTestProviderProfile(452-502)
🪛 Gitleaks (8.27.2)
controllers/sender/sender_test.go
255-255: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (1)
controllers/sender/sender_test.go (1)
15-15: Good addition: httpmock import is appropriate.
No issues with the import itself.
- Added indirect dependencies for miniredis and gopher-lua in go.mod and go.sum. - Refactored profile update logic to streamline error handling and improve validation checks for FeeAddress and HostIdentifier. - Enhanced transaction handling for currency availability updates within the profile update process.
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 (3)
controllers/accounts/profile.go (3)
72-77: Open transaction may leak on early returns; add deferred rollback.Many early returns occur after writes. Ensure rollback on all error paths.
tx, err := storage.Client.Tx(ctx) if err != nil { u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to update profile", nil) return } +defer func() { _ = tx.Rollback() }()
363-399: Pre-validate unique constraint on network when updating.If network changes to one already configured, fail validation early instead of 500 on commit.
existingToken, err := storage.Client.ProviderOrderToken. Query(). Where( providerordertoken.HasTokenWith(token.IDEQ(providerToken.ID)), providerordertoken.HasProviderWith(providerprofile.IDEQ(provider.ID)), providerordertoken.HasCurrencyWith(fiatcurrency.IDEQ(currency.ID)), providerordertoken.NetworkEQ(tokenPayload.Network), ). WithCurrency(). Only(ctx) isUpdate := err == nil if err != nil && !ent.IsNotFound(err) { ... } + // If updating and changing network, ensure target network doesn't already exist + if isUpdate && existingToken.Network != tokenPayload.Network { + dup, derr := storage.Client.ProviderOrderToken. + Query(). + Where( + providerordertoken.HasTokenWith(token.IDEQ(providerToken.ID)), + providerordertoken.HasProviderWith(providerprofile.IDEQ(provider.ID)), + providerordertoken.HasCurrencyWith(fiatcurrency.IDEQ(currency.ID)), + providerordertoken.NetworkEQ(tokenPayload.Network), + ).Exist(ctx) + if derr != nil { + u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to update profile", nil) + return + } + if dup { + validationErrors = append(validationErrors, types.ErrorData{ + Field: "Tokens", + Message: fmt.Sprintf("Token already configured on network %s for %s", tokenPayload.Network, tokenPayload.Symbol), + }) + continue + } + }
217-221: Allow omitting RateSlippage in ProviderOrderTokenPayload
In thetypes.ProviderOrderTokenPayloadstruct, update theRateSlippagebinding tag to includeomitemptyso that omitting the field (or providing zero) won’t trigger a validation error:- RateSlippage decimal.Decimal `json:"rateSlippage" binding:"gte=0.1"` + RateSlippage decimal.Decimal `json:"rateSlippage" binding:"omitempty,gte=0.1"`
♻️ Duplicate comments (2)
controllers/sender/sender_test.go (2)
237-245: Use regex URL matching for responders (handles query params/variants).Switch to regex-based matching to avoid brittle exact matches.
- httpmock.RegisterResponder("POST", "https://engine.thirdweb.com/v1/accounts", + httpmock.RegisterResponder("POST", `=~^https://engine\.thirdweb\.com/v1/accounts(\?.*)?$`, func(r *http.Request) (*http.Response, error) { return httpmock.NewJsonResponse(200, map[string]interface{}{ "result": map[string]interface{}{ "smartAccountAddress": "0x1234567890123456789012345678901234567890", }, }) }, ) - httpmock.RegisterResponder("POST", "https://1.insight.thirdweb.com/v1/webhooks", + httpmock.RegisterResponder("POST", `=~^https://[0-9]+\.insight\.thirdweb\.com/v1/webhooks(\?.*)?$`, func(r *http.Request) (*http.Response, error) { return httpmock.NewJsonResponse(200, map[string]interface{}{ "data": map[string]interface{}{ - "id": "webhook_123456789", - "webhook_secret": "secret_123456789", + "id": "wh_test_123456789", + "webhook_secret": "test_secret_123456789", // gitleaks:allow - test fixture }, }) }, )Also applies to: 248-257
252-254: Silence Gitleaks on test secret.Mark as allowlisted fixture to avoid false positives.
- "webhook_secret": "secret_123456789", + "webhook_secret": "test_secret_123456789", // gitleaks:allow - test fixture
🧹 Nitpick comments (5)
controllers/sender/sender_test.go (3)
233-235: Reset httpmock after test to avoid bleed.Use DeactivateAndReset to clear responders between subtests.
- httpmock.ActivateNonDefault(http.DefaultClient) - defer httpmock.Deactivate() + httpmock.ActivateNonDefault(http.DefaultClient) + defer httpmock.DeactivateAndReset()
202-208: Close Redis client to avoid resource leak.Close the client alongside miniredis shutdown.
mr, err := miniredis.Run() assert.NoError(t, err) defer mr.Close() db.RedisClient = redis.NewClient(&redis.Options{Addr: mr.Addr()}) +defer db.RedisClient.Close()
280-287: Auth header inconsistency: use HMAC for InitiatePaymentOrder too.Align with other requests to exercise the HMAC path.
- headers := map[string]string{ - "API-Key": testCtx.apiKey.ID.String(), - } + ts := time.Now().Unix() + sig := token.GenerateHMACSignature(map[string]interface{}{"timestamp": ts}, testCtx.apiKeySecret) + headers := map[string]string{ + "Authorization": "HMAC " + testCtx.apiKey.ID.String() + ":" + sig, + } - res, err := test.PerformRequest(t, "POST", "/sender/orders", payload, headers, router) + res, err := test.PerformRequest(t, "POST", fmt.Sprintf("/sender/orders?timestamp=%v", ts), payload, headers, router)controllers/accounts/profile.go (2)
347-361: Prefer exact decimals over floats for thresholds.Avoid float constructors to prevent rounding artifacts.
- if tokenPayload.RateSlippage.IsZero() { + if tokenPayload.RateSlippage.IsZero() { tokenPayload.RateSlippage = decimal.NewFromFloat(0) - } else if tokenPayload.RateSlippage.LessThan(decimal.NewFromFloat(0.1)) { + } else if tokenPayload.RateSlippage.LessThan(decimal.RequireFromString("0.1")) { ... - } else if rate.Mul(tokenPayload.RateSlippage.Div(decimal.NewFromFloat(100))).GreaterThan(currency.MarketRate.Mul(decimal.NewFromFloat(0.05))) { + } else if rate.Mul(tokenPayload.RateSlippage.Div(decimal.RequireFromString("100"))).GreaterThan(currency.MarketRate.Mul(decimal.RequireFromString("0.05"))) {
565-577: Include network in error logs for token ops.Add network to fields for faster diagnosis.
logger.WithFields(logger.Fields{ "Error": fmt.Sprintf("%v", err), "Token": op.TokenPayload.Symbol, "Currency": payload.Currency, + "Network": op.TokenPayload.Network, }).Errorf("Failed to update token during update")(Apply similarly in the create path.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
controllers/accounts/profile.go(10 hunks)controllers/sender/sender_test.go(4 hunks)go.mod(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
controllers/accounts/profile.go (5)
utils/http.go (1)
APIResponse(23-29)types/types.go (3)
ErrorData(565-568)ProviderOrderTokenPayload(252-262)SenderOrderTokenResponse(292-298)utils/utils.go (3)
IsValidEthereumAddress(420-424)IsValidHttpsUrl(723-737)AbsPercentageDeviation(202-209)ent/providerordertoken/providerordertoken.go (2)
ConversionRateType(117-117)ConversionRateTypeFloating(122-122)ent/ent.go (1)
IsNotFound(257-263)
controllers/sender/sender_test.go (3)
storage/redis.go (1)
RedisClient(13-13)utils/token/token.go (1)
GenerateHMACSignature(103-109)utils/test/test.go (1)
PerformRequest(14-25)
🪛 Gitleaks (8.27.2)
controllers/sender/sender_test.go
253-253: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (7)
go.mod (1)
36-36: Test-only deps verified:miniredisandgopher-luaare only imported in*_test.go(no non-test imports found via ripgrep), andgo mod whyshows they’re pulled in solely by tests.go.modentry is correct.controllers/accounts/profile.go (6)
84-86: Domain whitelist update guard LGTM.
231-242: Pre-flight HostIdentifier validation LGTM.
334-344: Market deviation check LGTM.Good to compute absolute percent deviation against configurable threshold.
530-557: Bucket intersection logic LGTM.Switch to MinAmount≤maxConverted AND MaxAmount≥minConverted is correct.
589-589: Success response LGTM.
638-646: Fix length source for tokens payload LGTM.
…e rate slippage validation
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 (2)
controllers/accounts/profile.go (2)
71-77: Open transaction can leak on early returns in UpdateSenderProfile. Add deferred rollback.Several error paths return after
Tx(ctx)without rollback. Add a deferred rollback immediately after starting the transaction.Apply:
tx, err := storage.Client.Tx(ctx) if err != nil { u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to update profile", nil) return } +defer func() { _ = tx.Rollback() }()
326-333: Add Min/Max order amount sanity check.Validate
MinOrderAmount <= MaxOrderAmountbefore bucket computations to avoid inverted ranges.Apply near rate computation:
if tokenPayload.ConversionRateType == providerordertoken.ConversionRateTypeFixed { rate = tokenPayload.FixedConversionRate } else { rate = currency.MarketRate.Add(tokenPayload.FloatingConversionRate) } + if tokenPayload.MinOrderAmount.GreaterThan(tokenPayload.MaxOrderAmount) { + validationErrors = append(validationErrors, types.ErrorData{ + Field: "Tokens", + Message: fmt.Sprintf("Min order amount cannot exceed max for %s", tokenPayload.Symbol), + }) + continue + }Also applies to: 334-344
♻️ Duplicate comments (2)
controllers/accounts/profile.go (2)
110-129: Validate RefundAddress the same way as FeeAddress (per-network).Currently only
FeeAddressis validated. Extend toRefundAddressto prevent bad addresses being saved.Apply:
if strings.HasPrefix(address.Network, "tron") { feeAddressIsValid := u.IsValidTronAddress(address.FeeAddress) if address.FeeAddress != "" && !feeAddressIsValid { u.APIResponse(ctx, http.StatusBadRequest, "error", "Failed to validate payload", []types.ErrorData{{ Field: "FeeAddress", Message: "Invalid Tron address", }}) return } + if address.RefundAddress != "" && !u.IsValidTronAddress(address.RefundAddress) { + u.APIResponse(ctx, http.StatusBadRequest, "error", "Failed to validate payload", []types.ErrorData{{ + Field: "RefundAddress", + Message: "Invalid Tron address", + }}) + return + } networksToTokenId[address.Network] = 0 } else { feeAddressIsValid := u.IsValidEthereumAddress(address.FeeAddress) if address.FeeAddress != "" && !feeAddressIsValid { u.APIResponse(ctx, http.StatusBadRequest, "error", "Failed to validate payload", []types.ErrorData{{ Field: "FeeAddress", Message: "Invalid Ethereum address", }}) return } + if address.RefundAddress != "" && !u.IsValidEthereumAddress(address.RefundAddress) { + u.APIResponse(ctx, http.StatusBadRequest, "error", "Failed to validate payload", []types.ErrorData{{ + Field: "RefundAddress", + Message: "Invalid Ethereum address", + }}) + return + } networksToTokenId[address.Network] = 0 }
386-389: Preserve-if-omitted path conflicts with required tag. Remove or revert types tag.This path won’t execute if
rateSlippageis required. Either remove it (if required stays) or switch the types tag toomitempty.Apply (if keeping required):
- // If updating, preserve existing rate slippage if not provided - if isUpdate && tokenPayload.RateSlippage.IsZero() && existingToken.RateSlippage.GreaterThan(decimal.NewFromFloat(0)) { - tokenPayload.RateSlippage = existingToken.RateSlippage - }
🧹 Nitpick comments (2)
controllers/accounts/profile.go (2)
334-344: Prefer exact decimal constants over floats.Avoid
decimal.NewFromFloatfor constants to prevent representation issues.Apply:
- percentDeviation := u.AbsPercentageDeviation(currency.MarketRate, rate) + percentDeviation := u.AbsPercentageDeviation(currency.MarketRate, rate)And elsewhere replace:
- decimal.NewFromFloat(0.1) + decimal.RequireFromString("0.1")
422-452: Use IDs when setting edges inside tx to avoid cross-driver entity capture.Use
SetProviderID(provider.ID)to avoid passing an entity loaded outside tx. Same forSetCurrencyID(curr.ID).Apply:
- _, err = tx.ProviderCurrencies.Create(). - SetProvider(provider). - SetCurrency(curr). + _, err = tx.ProviderCurrencies.Create(). + SetProviderID(provider.ID). + SetCurrencyID(curr.ID). SetAvailableBalance(decimal.Zero). SetTotalBalance(decimal.Zero). SetReservedBalance(decimal.Zero). SetIsAvailable(availabilityOp.isAvailable). Save(ctx)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
controllers/accounts/profile.go(10 hunks)types/types.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
types/types.go (1)
ent/providerordertoken/where.go (1)
RateSlippage(90-92)
controllers/accounts/profile.go (3)
utils/http.go (1)
APIResponse(23-29)types/types.go (2)
ErrorData(565-568)ProviderOrderTokenPayload(252-262)utils/utils.go (3)
IsValidEthereumAddress(420-424)IsValidHttpsUrl(723-737)AbsPercentageDeviation(202-209)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (6)
controllers/accounts/profile.go (6)
84-86: DomainWhitelist: good conditional update.
231-242: HTTPS-only HostIdentifier validation: solid.Good pre-transaction validation with clear error messaging.
363-374: Existing-token lookup correctly includes network in the uniqueness tuple.
537-542: Bucket overlap logic + dedup look good.Correct intersection check and dedup to prevent duplicate m2m edges.
Also applies to: 559-566
575-597: Transactional save/commit flow is solid.Single commit point with rollback on failure is correct.
648-656: Pre-sized tokensPayload based on fetched slice length: good.
…ing profile updates - Implemented a check to ensure that when updating a provider profile, the target network does not already have an existing token configuration. - Enhanced error handling to return appropriate responses when a duplicate token is detected or if an error occurs during the validation process.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/accounts/profile.go (1)
71-78: Missing rollback on early returns in UpdateSenderProfile leaves open transactions.Any error after Tx() returns causes an early return without rollback. Add a deferred rollback to guarantee cleanup.
Apply:
tx, err := storage.Client.Tx(ctx) if err != nil { u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to update profile", nil) return } +defer func() { _ = tx.Rollback() }() update := tx.SenderProfile.Update().Where(senderprofile.IDEQ(sender.ID))
♻️ Duplicate comments (5)
controllers/accounts/profile.go (5)
110-130: Validate RefundAddress per network (Tron/EVM) same as FeeAddress.Currently only FeeAddress is validated. Add RefundAddress checks to prevent bad returns.
Apply:
if strings.HasPrefix(address.Network, "tron") { feeAddressIsValid := u.IsValidTronAddress(address.FeeAddress) if address.FeeAddress != "" && !feeAddressIsValid { u.APIResponse(ctx, http.StatusBadRequest, "error", "Failed to validate payload", []types.ErrorData{{ Field: "FeeAddress", Message: "Invalid Tron address", }}) return } + if address.RefundAddress != "" && !u.IsValidTronAddress(address.RefundAddress) { + u.APIResponse(ctx, http.StatusBadRequest, "error", "Failed to validate payload", []types.ErrorData{{ + Field: "RefundAddress", + Message: "Invalid Tron address", + }}) + return + } networksToTokenId[address.Network] = 0 } else { feeAddressIsValid := u.IsValidEthereumAddress(address.FeeAddress) if address.FeeAddress != "" && !feeAddressIsValid { u.APIResponse(ctx, http.StatusBadRequest, "error", "Failed to validate payload", []types.ErrorData{{ Field: "FeeAddress", Message: "Invalid Ethereum address", }}) return } + if address.RefundAddress != "" && !u.IsValidEthereumAddress(address.RefundAddress) { + u.APIResponse(ctx, http.StatusBadRequest, "error", "Failed to validate payload", []types.ErrorData{{ + Field: "RefundAddress", + Message: "Invalid Ethereum address", + }}) + return + } networksToTokenId[address.Network] = 0 }
346-361: Slippage: remove dead zero-default branch and use exact decimals (no floats).Field is required (gte=0.1), so zero-branch is dead. Also avoid NewFromFloat to prevent precision drift.
Apply:
- // Handle slippage validation - if tokenPayload.RateSlippage.IsZero() { - tokenPayload.RateSlippage = decimal.NewFromFloat(0) - } else if tokenPayload.RateSlippage.LessThan(decimal.NewFromFloat(0.1)) { + // Handle slippage validation + if tokenPayload.RateSlippage.LessThan(decimal.RequireFromString("0.1")) { validationErrors = append(validationErrors, types.ErrorData{ Field: "Tokens", Message: fmt.Sprintf("Rate slippage cannot be less than 0.1%% for %s", tokenPayload.Symbol), }) continue - } else if rate.Mul(tokenPayload.RateSlippage.Div(decimal.NewFromFloat(100))).GreaterThan(currency.MarketRate.Mul(decimal.NewFromFloat(0.05))) { + } else if rate.Mul(tokenPayload.RateSlippage.Div(decimal.RequireFromString("100"))). + GreaterThan(currency.MarketRate.Mul(decimal.RequireFromString("0.05"))) { validationErrors = append(validationErrors, types.ErrorData{ Field: "Tokens", Message: fmt.Sprintf("Rate slippage is too high for %s", tokenPayload.Symbol), }) continue } - // If updating, preserve existing rate slippage if not provided - if isUpdate && tokenPayload.RateSlippage.IsZero() && existingToken.RateSlippage.GreaterThan(decimal.NewFromFloat(0)) { - tokenPayload.RateSlippage = existingToken.RateSlippage - } + // Slippage is required; no preservation branch neededAlso applies to: 409-412
498-510: Good fix: token updates now use the transaction-bound client.This enforces all-or-nothing semantics as intended.
557-566: Bucket selection now intersects ranges (correct).The AND of MinAmountLTE(max) and MaxAmountGTE(min) properly finds overlapping buckets.
582-596: Deduping buckets before AddProvisionBuckets avoids duplicate M2M edges.Solid fix.
🧹 Nitpick comments (4)
controllers/accounts/profile.go (4)
302-325: Avoid N× repeated currency lookups; fetch once before token loop.Currency is shared across tokens (payload.Currency). Cache it outside the loop to cut query overhead and reduce TOCTOU risk.
Apply within this block:
- // Ensure rate is within allowed deviation from the market rate - currency, err := storage.Client.FiatCurrency.Query(). - Where( - fiatcurrency.IsEnabledEQ(true), - fiatcurrency.CodeEQ(payload.Currency), - ). - Only(ctx) - if err != nil { - if ent.IsNotFound(err) { - validationErrors = append(validationErrors, types.ErrorData{ - Field: "Currency", - Message: "Currency not supported", - }) - } else { - ... - } - continue - } + // currency pre-fetched before the loop; use it hereAnd add before the for-loop (outside this range):
// Pre-fetch currency once currency, err := storage.Client.FiatCurrency. Query(). Where(fiatcurrency.IsEnabledEQ(true), fiatcurrency.CodeEQ(payload.Currency)). Only(ctx) if err != nil { if ent.IsNotFound(err) { u.APIResponse(ctx, http.StatusBadRequest, "error", "Currency not supported", []types.ErrorData{{Field: "Currency", Message: "Currency not supported"}}) } else { u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to update profile", nil) } return }
386-407: Unreachable branch: update-and-change-network check can never trigger.You set isUpdate from a query that already filters by NetworkEQ(tokenPayload.Network). Thus existingToken.Network != tokenPayload.Network is impossible. Decide intent:
- If you support changing network on an existing record, drop the NetworkEQ filter in the existence query and keep this check.
- If you do not support changing network (allow multi-network via create), delete this block.
Do you want me to adjust the query to ignore network when detecting “existing” and handle network-migration as an update, or remove the dead code?
436-444: Consolidate transaction cleanup: rely on one deferred rollback.You already defer tx.Rollback(). The explicit Rollback() calls inside error paths and after Commit add noise and can log “already closed” errors.
Apply:
defer func() { _ = tx.Rollback() }() ... - if err != nil { - if rollbackErr := tx.Rollback(); rollbackErr != nil { - logger.Errorf("Failed to rollback transaction: %v", rollbackErr) - } - ... - return - } + if err != nil { + ... + return + } ... - if err != nil { - if rollbackErr := tx.Rollback(); rollbackErr != nil { - logger.Errorf("Failed to rollback transaction: %v", rollbackErr) - } - ... - return - } + if err != nil { + ... + return + } ... - if err != nil { - if rollbackErr := tx.Rollback(); rollbackErr != nil { - logger.Errorf("Failed to rollback transaction: %v", rollbackErr) - } - ... - return - } + if err != nil { + ... + return + } ... - if err := tx.Commit(); err != nil { - if rollbackErr := tx.Rollback(); rollbackErr != nil { - logger.Errorf("Failed to rollback transaction: %v", rollbackErr) - } + if err := tx.Commit(); err != nil { logger.Errorf("Failed to commit transaction: %v", err) u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to update profile", nil) return }Also applies to: 512-514, 540-542, 601-603, 613-616
460-467: Prefer SetProviderID over SetProvider inside tx to avoid cross-client entity confusion.Functionally equivalent, but ID setter avoids passing a non-tx entity handle.
Apply:
- _, err = tx.ProviderCurrencies.Create(). - SetProvider(provider). + _, err = tx.ProviderCurrencies.Create(). + SetProviderID(provider.ID). SetCurrency(curr).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
controllers/accounts/profile.go(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/accounts/profile.go (5)
utils/http.go (1)
APIResponse(23-29)types/types.go (3)
ErrorData(565-568)ProviderOrderTokenPayload(252-262)SenderOrderTokenResponse(292-298)utils/utils.go (3)
IsValidEthereumAddress(420-424)IsValidHttpsUrl(723-737)AbsPercentageDeviation(202-209)ent/schema/providerordertoken.go (5)
ProviderOrderToken(12-14)ProviderOrderToken(17-21)ProviderOrderToken(24-41)ProviderOrderToken(44-59)ProviderOrderToken(62-67)ent/providerordertoken.go (2)
ProviderOrderToken(21-52)ProviderOrderToken(101-124)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (3)
controllers/accounts/profile.go (3)
84-86: DomainWhitelist: conditional update is correct.Only mutating when the field is provided avoids accidental clearing. LGTM.
231-242: HTTPS-only HostIdentifier validation is a good hardening.Nice improvement and consistent error shape.
671-671: tokensPayload length derived from query result is correct.Prevents length mismatches when edges aren’t eagerly loaded on sender.
- Add sqlite3 driver import to priority_queue_test.go to fix SQL driver error - Replace Redis dependency with miniredis in-memory server for CI compatibility - Remove blockchain RPC dependency by creating networks/tokens directly in database - Fix insufficient balance errors by setting adequate provider balances (100,000) - Create TestPriorityQueueService to mock provider API calls and bypass schema issues - Add enhanced error logging to sender tests for better CI debugging - Ensure all tests pass in both local and CI environments
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/priority_queue_test.go (1)
472-499: Use t.Cleanup and avoid log.Fatal in responders
- Prefer t.Cleanup(httpmock.DeactivateAndReset) over defer to avoid interacting with outer activation and ensure reset.
- Replace log.Fatal (which exits the process) with t.Fatalf to fail the subtest only.
- httpmock.Activate() - defer httpmock.Deactivate() + httpmock.Activate() + t.Cleanup(httpmock.DeactivateAndReset) @@ - bytes, err := io.ReadAll(r.Body) - if err != nil { - log.Fatal(err) - } + bytes, err := io.ReadAll(r.Body) + if err != nil { + t.Fatalf("read request body: %v", err) + }controllers/sender/sender_test.go (1)
280-282: Transition from API-Key to HMAC-based authorization pattern is incompleteThe test still uses the deprecated
API-Keyheader instead of the new HMAC-basedAuthorizationheader pattern that's used throughout the rest of the test file. This inconsistency could cause authentication failures.Apply this diff to use HMAC-based authorization:
+payload := map[string]interface{}{ + "amount": "100", + "token": testCtx.token.Symbol, + "rate": "750", + "network": network.Identifier, + "recipient": map[string]interface{}{ + "institution": "MOMONGPC", + "accountIdentifier": "1234567890", + "accountName": "John Doe", + "memo": "Shola Kehinde - rent for May 2021", + }, + "reference": "12kjdf-kjn33_REF", +} + +sig := token.GenerateHMACSignature(payload, testCtx.apiKeySecret) + headers := map[string]string{ - "API-Key": testCtx.apiKey.ID.String(), + "Authorization": "HMAC " + testCtx.apiKey.ID.String() + ":" + sig, } - -res, err := test.PerformRequest(t, "POST", "/sender/orders", payload, headers, router)
♻️ Duplicate comments (1)
controllers/sender/sender_test.go (1)
247-257: Apply regex matching and mark test secret to avoid security scan false positiveThe webhook URL responder should use regex matching for robustness, and the test secret needs a Gitleaks allowlist comment to silence the false positive security scan.
Apply this diff:
-httpmock.RegisterResponder("POST", "https://1.insight.thirdweb.com/v1/webhooks", +httpmock.RegisterResponder("POST", `=~^https://[0-9]+\.insight\.thirdweb\.com/v1/webhooks(\?.*)?$`, func(r *http.Request) (*http.Response, error) { return httpmock.NewJsonResponse(200, map[string]interface{}{ "data": map[string]interface{}{ - "id": "webhook_123456789", - "webhook_secret": "secret_123456789", + "id": "wh_test_123456789", + "webhook_secret": "test_secret_123456789", // gitleaks:allow - test fixture }, }) }, )
🧹 Nitpick comments (9)
services/priority_queue_test.go (9)
54-73: Guard against nil edges in the test stuborder.ProvisionBucket or order.ProvisionBucket.Edges.Currency can be nil if the caller didn’t preload edges. Add a fast-fail to avoid nil deref in tests.
func (s *TestPriorityQueueService) sendOrderRequest(ctx context.Context, order types.LockPaymentOrderFields) error { - // Mock successful balance reservation and provider notification - bucketCurrency := order.ProvisionBucket.Edges.Currency + if order.ProvisionBucket == nil || order.ProvisionBucket.Edges.Currency == nil { + return fmt.Errorf("sendOrderRequest(test): missing ProvisionBucket or Currency edge") + } + bucketCurrency := order.ProvisionBucket.Edges.Currency amount := order.Amount.Mul(order.Rate).Round(int32(bucketCurrency.Decimals))
75-80: Prefer DI-friendly constructor for testsNewTestPriorityQueueService only wraps NewPriorityQueueService. If you move to DI (e.g., an orderRequester func field) you can set the stub here and avoid embedding pitfalls.
I can supply the refactor patch if you want to proceed.
87-127: Seeding via upsert looks good; minor polishLGTM overall. Consider threading a ctx param into setupForPQ and using it here rather than context.Background() for consistency with ent.
298-299: Service instance creationGiven the embedding caveat, be aware that internal calls still hit real sendOrderRequest unless you refactor to DI.
Would you like me to sketch the minimal DI change across PriorityQueueService and this test?
323-331: Redis key format couplingKey format "bucket_%s_%s_%s" is duplicated here. If the prod code changes the format, this test will silently drift. Consider exposing a small helper to format the key and reuse it in tests.
403-408: Avoid strict float equality in rate assertionPrefer an epsilon or ensure expected is first arg.
- assert.Equal(t, _rate, float64(100)) + assert.InDelta(t, 100.0, _rate, 1e-9)
442-453: Redundant httpmock activation — this stubbed sendOrderRequest never does HTTPYou activate httpmock and register a responder, but the test override of sendOrderRequest does no network I/O. Remove this block to avoid confusion and nested activations below.
- // Setup httpmock for sendOrderRequest - httpmock.Activate() - defer httpmock.DeactivateAndReset() - - httpmock.RegisterResponder("POST", testCtxForPQ.publicProviderProfile.HostIdentifier+"/new_order", - func(req *http.Request) (*http.Response, error) { - return httpmock.NewJsonResponse(200, map[string]interface{}{ - "status": "success", - "message": "Order processed successfully", - }) - })
271-271: Test name nitConsider renaming TestPriorityQueueTest to TestPriorityQueue to avoid the redundant suffix.
386-399: Drop or adjust the Network field suggestion
TheLockPaymentOrderFieldsstruct already definesNetwork, butAssignLockPaymentOrderignores it and always usesorder.Token.Edges.Network. AddingNetwork: testCtxForPQ.token.Edges.Networkin this test won’t affect any logic. Either refactorAssignLockPaymentOrderto readorder.Networkor remove this diff.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
controllers/sender/sender_test.go(6 hunks)services/priority_queue_test.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
services/priority_queue_test.go (6)
services/priority_queue.go (2)
PriorityQueueService(31-33)NewPriorityQueueService(36-40)types/types.go (1)
LockPaymentOrderFields(335-356)ent/token/where.go (3)
Decimals(79-81)ID(14-16)IDEQ(19-21)utils/logger/logger.go (4)
WithFields(76-109)Fields(73-73)Infof(125-130)Errorf(141-146)ent/migrate/migrate.go (3)
Create(49-55)Schema(36-38)WithGlobalUniqueID(19-19)storage/redis.go (1)
RedisClient(13-13)
controllers/sender/sender_test.go (3)
storage/redis.go (1)
RedisClient(13-13)utils/token/token.go (1)
GenerateHMACSignature(103-109)utils/test/test.go (1)
PerformRequest(14-25)
🪛 Gitleaks (8.27.2)
controllers/sender/sender_test.go
253-253: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (7)
services/priority_queue_test.go (3)
276-280: Migrations in testsRunning client.Schema.Create with migrate.WithGlobalUniqueID(true) is appropriate for a fresh in-memory SQLite.
281-289: In-memory Redis setup is cleanminiredis + redis client wiring looks good and isolated.
471-505: Keep httpmock activation scoping flatGiven the earlier block is removed, this subtest’s activation is now the only one. Good. Ensure no other tests run concurrently depend on http.DefaultClient; if they do, consider ActivateNonDefault with a dedicated client.
controllers/sender/sender_test.go (4)
291-293: LGTM!The additional debug logging for request payload and headers will help troubleshoot test failures by providing more context about the request being made.
312-321: LGTM!The improved error handling and type checking for the payment order ID extraction makes the test more robust and provides better diagnostics when the response format is unexpected.
341-346: LGTM!The transition to HMAC-based authentication for the transaction logs check is consistent with the overall authentication pattern changes in the test file.
348-348: LGTM!The timestamp parameter is correctly included in the URL query string, matching the HMAC signature payload for proper authentication.
| // TestPriorityQueueService extends the original service with test-specific overrides | ||
| type TestPriorityQueueService struct { | ||
| *PriorityQueueService | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Embedding won’t mock internal calls to sendOrderRequest
Go method calls inside PriorityQueueService (receiver typed as *PriorityQueueService) will statically bind to that type’s methods, not the wrapper’s. Calls like s.sendOrderRequest from within PriorityQueueService will bypass this test override. Use dependency injection (an interface or function field) on the service instead of embedding if you intend to stub internal calls.
Run to confirm call sites and receiver types:
Follow-up: If you want, I can generate a small refactor that injects an OrderRequester interface and updates tests accordingly.
🏁 Script executed:
#!/bin/bash
rg -nP -C3 $'func\\s*\\(\\s*\\*PriorityQueueService\\s*\\)\\s*sendOrderRequest\\b' --type=go
rg -nP -C3 $'\\bsendOrderRequest\\s*\\(' services --type=goLength of output: 2606
sendOrderRequest override via embedding won’t intercept internal calls
PriorityQueueService’s internal calls to s.sendOrderRequest are statically bound to *PriorityQueueService, so your TestPriorityQueueService.sendOrderRequest override is never invoked. Extract sendOrderRequest behind an injected interface or function field and update both the service and its tests to use that abstraction.
🤖 Prompt for AI Agents
In services/priority_queue_test.go around lines 49 to 53, the test's embedding
override of sendOrderRequest on TestPriorityQueueService does not intercept
internal calls because PriorityQueueService calls are statically bound; to fix
this, refactor PriorityQueueService to depend on an abstraction (either an
interface with a sendOrderRequest method or a sendOrderRequest function field)
and replace direct method calls with calls through that field/interface; update
constructors and production code to set the real implementation and update tests
to inject a test double (either a test struct implementing the interface or a
lambda assigned to the function field) so TestPriorityQueueService can intercept
and assert sendOrderRequest behavior.
| // Update ProviderCurrencies with sufficient balance for the publicProviderProfile | ||
| _, err = db.Client.ProviderCurrencies. | ||
| Update(). | ||
| Where(providercurrencies.HasProviderWith(providerprofile.IDEQ(publicProviderProfile.ID))). | ||
| Where(providercurrencies.HasCurrencyWith(fiatcurrency.IDEQ(currency.ID))). | ||
| SetAvailableBalance(decimal.NewFromFloat(100000)). // Set sufficient balance | ||
| SetTotalBalance(decimal.NewFromFloat(100000)). | ||
| Save(context.Background()) | ||
| if err != nil { | ||
| return fmt.Errorf("UpdateProviderCurrencies.publicProvider: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update() may silently affect 0 rows; assert affected > 0
If no ProviderCurrencies row exists, Update().Save(ctx) returns (0, nil), masking setup issues and leading to insufficient balance later. Check affected rows and fail fast.
- _, err = db.Client.ProviderCurrencies.
+ n, err := db.Client.ProviderCurrencies.
Update().
Where(providercurrencies.HasProviderWith(providerprofile.IDEQ(publicProviderProfile.ID))).
Where(providercurrencies.HasCurrencyWith(fiatcurrency.IDEQ(currency.ID))).
SetAvailableBalance(decimal.NewFromFloat(100000)). // Set sufficient balance
SetTotalBalance(decimal.NewFromFloat(100000)).
Save(context.Background())
- if err != nil {
- return fmt.Errorf("UpdateProviderCurrencies.publicProvider: %w", err)
- }
+ if err != nil {
+ return fmt.Errorf("UpdateProviderCurrencies.publicProvider: %w", err)
+ }
+ if n == 0 {
+ return fmt.Errorf("UpdateProviderCurrencies.publicProvider: no rows updated for provider=%s currency=%s", publicProviderProfile.ID, currency.Code)
+ }📝 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.
| // Update ProviderCurrencies with sufficient balance for the publicProviderProfile | |
| _, err = db.Client.ProviderCurrencies. | |
| Update(). | |
| Where(providercurrencies.HasProviderWith(providerprofile.IDEQ(publicProviderProfile.ID))). | |
| Where(providercurrencies.HasCurrencyWith(fiatcurrency.IDEQ(currency.ID))). | |
| SetAvailableBalance(decimal.NewFromFloat(100000)). // Set sufficient balance | |
| SetTotalBalance(decimal.NewFromFloat(100000)). | |
| Save(context.Background()) | |
| if err != nil { | |
| return fmt.Errorf("UpdateProviderCurrencies.publicProvider: %w", err) | |
| } | |
| // Update ProviderCurrencies with sufficient balance for the publicProviderProfile | |
| n, err := db.Client.ProviderCurrencies. | |
| Update(). | |
| Where(providercurrencies.HasProviderWith(providerprofile.IDEQ(publicProviderProfile.ID))). | |
| Where(providercurrencies.HasCurrencyWith(fiatcurrency.IDEQ(currency.ID))). | |
| SetAvailableBalance(decimal.NewFromFloat(100000)). // Set sufficient balance | |
| SetTotalBalance(decimal.NewFromFloat(100000)). | |
| Save(context.Background()) | |
| if err != nil { | |
| return fmt.Errorf("UpdateProviderCurrencies.publicProvider: %w", err) | |
| } | |
| if n == 0 { | |
| return fmt.Errorf("UpdateProviderCurrencies.publicProvider: no rows updated for provider=%s currency=%s", publicProviderProfile.ID, currency.Code) | |
| } |
🤖 Prompt for AI Agents
In services/priority_queue_test.go around lines 188 to 199, the Update().Save
call may return (0, nil) when no rows match, silently masking setup failures;
change the code to capture the affected count and error from Save, and if
affected == 0 (or err != nil) return a descriptive error right away so the test
fails fast when no ProviderCurrencies row was updated.
| // Update ProviderCurrencies with sufficient balance for the privateProviderProfile | ||
| _, err = db.Client.ProviderCurrencies. | ||
| Update(). | ||
| Where(providercurrencies.HasProviderWith(providerprofile.IDEQ(privateProviderProfile.ID))). | ||
| Where(providercurrencies.HasCurrencyWith(fiatcurrency.IDEQ(currency.ID))). | ||
| SetAvailableBalance(decimal.NewFromFloat(100000)). // Set sufficient balance | ||
| SetTotalBalance(decimal.NewFromFloat(100000)). | ||
| Save(context.Background()) | ||
| if err != nil { | ||
| return fmt.Errorf("UpdateProviderCurrencies.privateProvider: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same issue for private provider currency update
Apply the same affected-rows check here.
- _, err = db.Client.ProviderCurrencies.
+ n, err := db.Client.ProviderCurrencies.
Update().
Where(providercurrencies.HasProviderWith(providerprofile.IDEQ(privateProviderProfile.ID))).
Where(providercurrencies.HasCurrencyWith(fiatcurrency.IDEQ(currency.ID))).
SetAvailableBalance(decimal.NewFromFloat(100000)). // Set sufficient balance
SetTotalBalance(decimal.NewFromFloat(100000)).
Save(context.Background())
- if err != nil {
- return fmt.Errorf("UpdateProviderCurrencies.privateProvider: %w", err)
- }
+ if err != nil {
+ return fmt.Errorf("UpdateProviderCurrencies.privateProvider: %w", err)
+ }
+ if n == 0 {
+ return fmt.Errorf("UpdateProviderCurrencies.privateProvider: no rows updated for provider=%s currency=%s", privateProviderProfile.ID, currency.Code)
+ }📝 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.
| // Update ProviderCurrencies with sufficient balance for the privateProviderProfile | |
| _, err = db.Client.ProviderCurrencies. | |
| Update(). | |
| Where(providercurrencies.HasProviderWith(providerprofile.IDEQ(privateProviderProfile.ID))). | |
| Where(providercurrencies.HasCurrencyWith(fiatcurrency.IDEQ(currency.ID))). | |
| SetAvailableBalance(decimal.NewFromFloat(100000)). // Set sufficient balance | |
| SetTotalBalance(decimal.NewFromFloat(100000)). | |
| Save(context.Background()) | |
| if err != nil { | |
| return fmt.Errorf("UpdateProviderCurrencies.privateProvider: %w", err) | |
| } | |
| // Update ProviderCurrencies with sufficient balance for the privateProviderProfile | |
| n, err := db.Client.ProviderCurrencies. | |
| Update(). | |
| Where(providercurrencies.HasProviderWith(providerprofile.IDEQ(privateProviderProfile.ID))). | |
| Where(providercurrencies.HasCurrencyWith(fiatcurrency.IDEQ(currency.ID))). | |
| SetAvailableBalance(decimal.NewFromFloat(100000)). // Set sufficient balance | |
| SetTotalBalance(decimal.NewFromFloat(100000)). | |
| Save(context.Background()) | |
| if err != nil { | |
| return fmt.Errorf("UpdateProviderCurrencies.privateProvider: %w", err) | |
| } | |
| if n == 0 { | |
| return fmt.Errorf("UpdateProviderCurrencies.privateProvider: no rows updated for provider=%s currency=%s", privateProviderProfile.ID, currency.Code) | |
| } |
🤖 Prompt for AI Agents
In services/priority_queue_test.go around lines 229 to 240, the
ProviderCurrencies update does not verify the number of affected rows; change
the Save call to capture the affected count (n, err := ... Save(ctx)), check err
as before, and then assert n == 1 (return a descriptive error if n != 1) so the
test fails if the update did not actually modify exactly one row.
- Add context field to error responses without exposing internal error details - Maintain detailed logging for internal debugging while keeping API responses clean - Help identify specific failure points in CI environment (address creation, validation, etc.) - Tests pass locally, improved error context will help debug CI-specific issues
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
controllers/sender/sender.go (5)
239-243: Log with fields and align message with new error context.Match the response context by adding structured fields; keeps payload as-is.
- logger.Errorf("Failed to fetch institution: %v", err) - u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to validate institution", map[string]interface{}{ - "context": "institution_fetch", - }) + logger.WithFields(logger.Fields{ + "context": "institution_fetch", + "code": payload.Recipient.Institution, + "err": err, + }).Errorf("Failed to fetch institution") + u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to validate institution", map[string]interface{}{ + "context": "institution_fetch", + })
353-357: Include rich context in queue rate error logs.Add fields for token, amount, fiat, market rate, and network to speed up incident debugging.
- if err != nil { - logger.Errorf("InitiatePaymentOrder.GetTokenRateFromQueue: %v", err) - u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to initiate payment order", map[string]interface{}{ - "context": "token_rate_queue", - }) + if err != nil { + logger.WithFields(logger.Fields{ + "context": "token_rate_queue", + "token": "USDT", + "amount": normalizedAmount, + "fiat": institutionObj.Edges.FiatCurrency.Code, + "market_rate": institutionObj.Edges.FiatCurrency.MarketRate, + "network": token.Edges.Network.Identifier, + "err": err, + }).Errorf("GetTokenRateFromQueue failed") + u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to initiate payment order", map[string]interface{}{ + "context": "token_rate_queue", + })
376-381: Use structured logging for Tron address creation failures.Mirror the response context in logs for correlation.
- if err != nil { - logger.Errorf("CreateTronAddress error: %v", err) - u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to initiate payment order", map[string]interface{}{ - "context": "create_tron_address", - }) + if err != nil { + logger.WithFields(logger.Fields{ + "context": "create_tron_address", + "network": payload.Network, + "err": err, + }).Errorf("CreateTronAddress error") + u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to initiate payment order", map[string]interface{}{ + "context": "create_tron_address", + })
405-411: Add context field to smart address creation logs.You already added response context; include it in logs too (keep existing fields).
- logger.WithFields(logger.Fields{ - "error": err, - "address": address, - }).Errorf("Failed to create receive address") + logger.WithFields(logger.Fields{ + "context": "create_smart_address", + "error": err, + "address": address, + "label": uniqueLabel, + }).Errorf("Failed to create receive address")
204-207: Add structured log fields for error contexts
Wrap the logger call withWithFields, including"context","reference", and the error for better observability; leave the API response payload unchanged. Centralize context literals into constants.// controllers/sender/sender.go: around line 204 logger.WithFields(logger.Fields{ "context": errCtxReferenceCheck, "reference": payload.Reference, "err": err, }).Errorf("Reference check error") u.APIResponse(ctx, http.StatusInternalServerError, "error", "Failed to initiate payment order", map[string]interface{}{ "context": errCtxReferenceCheck, })// centralize these in an appropriate package const ( errCtxReferenceCheck = "reference_check" errCtxInstitutionFetch = "institution_fetch" errCtxTokenRateQueue = "token_rate_queue" errCtxCreateTronAddr = "create_tron_address" errCtxCreateSmartAddr = "create_smart_address" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
controllers/sender/sender.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/sender/sender.go (2)
utils/logger/logger.go (1)
Errorf(141-146)utils/http.go (1)
APIResponse(23-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
… environment variables - Set ENGINE_BASE_URL to match HTTP mocks in sender test - Set THIRDWEB_SECRET_KEY for engine service authentication - Add proper environment variable cleanup in test teardown - Import os package for environment variable management - Fix create_smart_address context error in CI by ensuring engine service configuration matches test mocks
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
controllers/sender/sender_test.go (2)
535-543: Don’t silently return on parse errors; fail the test.Early returns hide failures.
Apply:
- firstOrderTimestamp, err := time.Parse(time.RFC3339Nano, data["orders"].([]interface{})[0].(map[string]interface{})["createdAt"].(string)) - if err != nil { - return - } + firstOrderTimestamp, err := time.Parse(time.RFC3339Nano, data["orders"].([]interface{})[0].(map[string]interface{})["createdAt"].(string)) + if err != nil { + t.Fatalf("failed to parse first order createdAt: %v", err) + } @@ - lastOrderTimestamp, err := time.Parse(time.RFC3339Nano, data["orders"].([]interface{})[len(data["orders"].([]interface{}))-1].(map[string]interface{})["createdAt"].(string)) - if err != nil { - return - } + lastOrderTimestamp, err := time.Parse(time.RFC3339Nano, data["orders"].([]interface{})[len(data["orders"].([]interface{}))-1].(map[string]interface{})["createdAt"].(string)) + if err != nil { + t.Fatalf("failed to parse last order createdAt: %v", err) + }
627-651: Replace early returns with assertions to surface setup failures.Returning on error causes false positives.
Apply:
- if err != nil { - return - } + assert.NoError(t, err) @@ - if err != nil { - return - } + assert.NoError(t, err) @@ - if err != nil { - return - } + assert.NoError(t, err) @@ - if err != nil { - return - } + assert.NoError(t, err)
♻️ Duplicate comments (1)
controllers/sender/sender_test.go (1)
245-266: Use regex URL matchers; avoid brittle exact-string responders. Also allowlist test secret.Exact strings break when query params or sharded hosts appear. Switch to regex matchers and annotate the test secret.
Apply:
- httpmock.RegisterResponder("POST", "https://engine.thirdweb.com/v1/accounts", + httpmock.RegisterResponder("POST", `=~^https://engine\.thirdweb\.com/v1/accounts(\?.*)?$`, func(r *http.Request) (*http.Response, error) { return httpmock.NewJsonResponse(200, map[string]interface{}{ "result": map[string]interface{}{ "smartAccountAddress": "0x1234567890123456789012345678901234567890", }, }) }, ) - - // Mock the engine service call for webhook creation - httpmock.RegisterResponder("POST", "https://1.insight.thirdweb.com/v1/webhooks", + // Mock the engine service call for webhook creation + httpmock.RegisterResponder("POST", `=~^https://[0-9]+\.insight\.thirdweb\.com/v1/webhooks(\?.*)?$`, func(r *http.Request) (*http.Response, error) { return httpmock.NewJsonResponse(200, map[string]interface{}{ "data": map[string]interface{}{ - "id": "webhook_123456789", - "webhook_secret": "secret_123456789", + "id": "wh_test_123456789", + "webhook_secret": "test_secret_123456789", // gitleaks:allow - test fixture }, }) }, )
🧹 Nitpick comments (5)
controllers/sender/sender_test.go (5)
233-239: Use t.Setenv and mark test secret as allowlisted.t.Setenv auto-cleans and is concurrency-safe; also silence Gitleaks on the known test secret.
Apply:
- // Set environment variables for engine service to match our mocks - os.Setenv("ENGINE_BASE_URL", "https://engine.thirdweb.com") - os.Setenv("THIRDWEB_SECRET_KEY", "test-secret-key") - defer func() { - os.Unsetenv("ENGINE_BASE_URL") - os.Unsetenv("THIRDWEB_SECRET_KEY") - }() + // Set environment variables for engine service to match our mocks + t.Setenv("ENGINE_BASE_URL", "https://engine.thirdweb.com") + t.Setenv("THIRDWEB_SECRET_KEY", "test-secret-key") // gitleaks:allow - test fixture
8-8: Drop unused import after switching to t.Setenv."os" becomes unused with the change above.
Apply:
- "os"
269-275: Avoid shadowing the imported network package.The local variable named network hides the ent/network import, reducing clarity.
Apply:
- network, err := db.Client.Network. + nw, err := db.Client.Network. Query(). - Where(network.IdentifierEQ(testCtx.networkIdentifier)). + Where(network.IdentifierEQ(testCtx.networkIdentifier)). Only(context.Background()) @@ - "network": network.Identifier, + "network": nw.Identifier, @@ - assert.Equal(t, data["transactionFee"], network.Fee.String()) + assert.Equal(t, data["transactionFee"], nw.Fee.String())Also applies to: 279-279, 347-347
315-319: Assert upstream call counts to the mocked services.Helps ensure the controller hits expected endpoints exactly once.
Apply:
assert.NotEmpty(t, data["validUntil"]) + // Verify expected external calls occurred exactly once + calls := httpmock.GetCallCountInfo() + assert.Equal(t, 1, calls[`POST =~^https://engine\.thirdweb\.com/v1/accounts(\?.*)?$`]) + assert.Equal(t, 1, calls[`POST =~^https://[0-9]+\.insight\.thirdweb\.com/v1/webhooks(\?.*)?$`])
241-244: Use DeactivateAndReset and register a NoResponder
Replace the existing Activate/Deactivate calls to ensure no mock state leaks between subtests and to fail fast on any unexpected HTTP request:- httpmock.ActivateNonDefault(http.DefaultClient) - defer httpmock.Deactivate() + httpmock.ActivateNonDefault(http.DefaultClient) + defer httpmock.DeactivateAndReset() + httpmock.RegisterNoResponder(httpmock.NewStringResponder(500, "unexpected HTTP call"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
controllers/sender/sender_test.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/sender/sender_test.go (3)
storage/redis.go (1)
RedisClient(13-13)utils/token/token.go (1)
GenerateHMACSignature(103-109)utils/test/test.go (1)
PerformRequest(14-25)
🪛 Gitleaks (8.27.2)
controllers/sender/sender_test.go
262-262: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (2)
controllers/sender/sender_test.go (2)
203-209: LGTM: miniredis-based Redis stub isolates tests.Good replacement for real Redis; simple, fast, and hermetic.
824-833: Verify fee formula vs. comment; numbers don’t align.Comment says “5% of 100 NGN / 950 ≈ 0.005 USD”, but code divides by 750 then 950. Confirm intended conversion.
Would you like me to adjust the expected value or the formula based on how the service computes USD fees (order rate vs. market rate)? I can update the test accordingly.
…ibility - Change from httpmock.ActivateNonDefault to httpmock.Activate for global coverage - Use httpmock.DeactivateAndReset for proper cleanup - Ensure third-party HTTP libraries like fastshot are properly mocked - Fix create_smart_address HTTP mocking in CI environment
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/sender/sender_test.go (1)
824-834: Incorrect expectedFee formula in sender stats test
The test’s expectedFee divides by both 750 and 950, but the Stats controller converts local NGN fees to USD by dividing only by the NGN market rate. Update the assertion to:expectedFee := decimal.NewFromFloat(5.0).Div(decimal.NewFromFloat(950.0))(or equivalently
decimal.NewFromFloat(100.0).Mul(decimal.NewFromFloat(0.05)).Div(decimal.NewFromFloat(950.0))) at controllers/sender/sender_test.go:829
♻️ Duplicate comments (1)
controllers/sender/sender_test.go (1)
246-254: Make URL matching resilient and silence Gitleaks on test secret.Use regex responders to ignore query params and subdomain variants. Also rename/annotate the test webhook secret to avoid CI leaks blocking the PR.
-// Mock the engine service call for receive address creation -httpmock.RegisterResponder("POST", "https://engine.thirdweb.com/v1/accounts", +// Mock the engine service call for receive address creation +httpmock.RegisterResponder("POST", `=~^https://engine\.thirdweb\.com/v1/accounts(\?.*)?$`, func(r *http.Request) (*http.Response, error) { return httpmock.NewJsonResponse(200, map[string]interface{}{ "result": map[string]interface{}{ "smartAccountAddress": "0x1234567890123456789012345678901234567890", }, }) }, ) -// Mock the engine service call for webhook creation -httpmock.RegisterResponder("POST", "https://1.insight.thirdweb.com/v1/webhooks", +// Mock the engine service call for webhook creation +httpmock.RegisterResponder("POST", `=~^https://[0-9]+\.insight\.thirdweb\.com/v1/webhooks(\?.*)?$`, func(r *http.Request) (*http.Response, error) { return httpmock.NewJsonResponse(200, map[string]interface{}{ "data": map[string]interface{}{ - "id": "webhook_123456789", - "webhook_secret": "secret_123456789", + "id": "wh_test_123456789", + "webhook_secret": "test_secret_123456789", // gitleaks:allow - test fixture }, }) }, )Optional: derive the base URL from ENGINE_BASE_URL and use regexp.QuoteMeta to build the pattern dynamically.
Also applies to: 256-266
🧹 Nitpick comments (5)
controllers/sender/sender_test.go (5)
203-209: Good switch to miniredis; close the client to avoid leaks.Add a defer to close db.RedisClient after the test to keep connections tidy.
db.RedisClient = redis.NewClient(&redis.Options{Addr: mr.Addr()}) +defer func() { _ = db.RedisClient.Close() }()
233-239: Use t.Setenv for env isolation.t.Setenv auto-restores values and avoids cross-test leakage; the manual unset defer can be dropped.
-// Set environment variables for engine service to match our mocks -os.Setenv("ENGINE_BASE_URL", "https://engine.thirdweb.com") -os.Setenv("THIRDWEB_SECRET_KEY", "test-secret-key") -defer func() { - os.Unsetenv("ENGINE_BASE_URL") - os.Unsetenv("THIRDWEB_SECRET_KEY") -}() +// Set environment variables for engine service to match our mocks +t.Setenv("ENGINE_BASE_URL", "https://engine.thirdweb.com") +t.Setenv("THIRDWEB_SECRET_KEY", "test-secret-key")
289-295: Align InitiatePaymentOrder auth with HMAC scheme used elsewhere.POST currently uses only API-Key; other endpoints use HMAC + timestamp. Recommend signing POST as well for consistency.
-headers := map[string]string{ - "API-Key": testCtx.apiKey.ID.String(), -} - -res, err := test.PerformRequest(t, "POST", "/sender/orders", payload, headers, router) +ts := time.Now().Unix() +sigPayload := map[string]interface{}{"timestamp": ts} +sig := token.GenerateHMACSignature(sigPayload, testCtx.apiKeySecret) +headers := map[string]string{ + "Authorization": "HMAC " + testCtx.apiKey.ID.String() + ":" + sig, +} +res, err := test.PerformRequest(t, "POST", fmt.Sprintf("/sender/orders?timestamp=%v", ts), payload, headers, router)If API-Key fallback is still supported, keep it alongside Authorization until the deprecation window closes.
Also applies to: 293-293
148-149: Avoid flakiness: increase validUntil from 5ms.5ms can expire mid-test on slow CI. Use a more generous window.
- SetValidUntil(time.Now().Add(time.Millisecond * 5)). + SetValidUntil(time.Now().Add(time.Minute)).Also applies to: 750-751
738-738: Remove stray assert on an unrelated err.This asserts the outer-scope err before any ops in this subtest; it’s meaningless and could hide issues.
-assert.NoError(t, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
controllers/sender/sender_test.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/sender/sender_test.go (3)
storage/redis.go (1)
RedisClient(13-13)utils/token/token.go (1)
GenerateHMACSignature(103-109)utils/test/test.go (1)
PerformRequest(14-25)
🪛 Gitleaks (8.27.2)
controllers/sender/sender_test.go
262-262: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (2)
controllers/sender/sender_test.go (2)
300-301: Helpful failure logs.The conditional request/headers logging will speed up triage when assertions fail.
321-329: Robust ID extraction LGTM.Good defensive checks before parsing UUID.
…t and implement two-phase provider profile updates (#516) * fix(schema): add network field to ProviderOrderToken unique constraint and implement two-phase provider profile updates - Update unique index to include network field alongside provider, token, and currency - Fixes bug preventing providers from supporting same token/currency on multiple networks - Resolves issue where provider profile updates failed for new network configurations - Migration included to update existing database constraint - Implement two-phase approach for provider profile updates: * Phase 1: Comprehensive validation of all tokens before database changes * Phase 2: Single transaction execution of all operations * Fixes silent failure bug where profile updates succeeded but token creation failed * Provides detailed validation error reporting for all issues at once * Ensures data consistency with all-or-nothing approach The previous constraint only allowed one ProviderOrderToken per (provider, token, currency) combination, but the application logic queries by (provider, token, currency, network). This mismatch caused 'provider does not support the selected token' errors when trying to configure the same token on different networks. BREAKING CHANGE: Database schema change requires migration to be applied * fix: improve transaction handling and bucket selection logic - Use UpdateOneID for transaction-bound token updates - Add backfill logic to migration before enforcing NOT NULL constraint - Fix bucket selection logic to use proper range intersection - Ensure providers are assigned to all buckets that overlap their range - Prevents migration failure on existing NULL network values - More efficient and safer database operations * fix: update profile validation messages and improve provider profile update logic - Modify validation messages to include context for TST in error responses. - Refactor provider profile update logic to ensure basic fields are set within the transaction. - Enhance error handling for validation failures to provide clearer feedback on issues encountered. - Introduce a more structured approach to setting profile fields during updates. * test: add provision bucket creation and update logic in provider tests * chore: update dependencies and improve profile update logic - Added indirect dependencies for miniredis and gopher-lua in go.mod and go.sum. - Refactored profile update logic to streamline error handling and improve validation checks for FeeAddress and HostIdentifier. - Enhanced transaction handling for currency availability updates within the profile update process. * fix: deduplicate provision buckets in profile update logic and enforce rate slippage validation * fix: add validation to prevent duplicate tokens on network change during profile updates - Implemented a check to ensure that when updating a provider profile, the target network does not already have an existing token configuration. - Enhanced error handling to return appropriate responses when a duplicate token is detected or if an error occurs during the validation process. * fix: resolve CI test failures in priority queue and sender tests - Add sqlite3 driver import to priority_queue_test.go to fix SQL driver error - Replace Redis dependency with miniredis in-memory server for CI compatibility - Remove blockchain RPC dependency by creating networks/tokens directly in database - Fix insufficient balance errors by setting adequate provider balances (100,000) - Create TestPriorityQueueService to mock provider API calls and bypass schema issues - Add enhanced error logging to sender tests for better CI debugging - Ensure all tests pass in both local and CI environments * improve: add context to error responses for better CI debugging - Add context field to error responses without exposing internal error details - Maintain detailed logging for internal debugging while keeping API responses clean - Help identify specific failure points in CI environment (address creation, validation, etc.) - Tests pass locally, improved error context will help debug CI-specific issues * fix: resolve sender test create_smart_address error by setting engine environment variables - Set ENGINE_BASE_URL to match HTTP mocks in sender test - Set THIRDWEB_SECRET_KEY for engine service authentication - Add proper environment variable cleanup in test teardown - Import os package for environment variable management - Fix create_smart_address context error in CI by ensuring engine service configuration matches test mocks * fix: enable global HTTP transport mocking for fastshot library compatibility - Change from httpmock.ActivateNonDefault to httpmock.Activate for global coverage - Use httpmock.DeactivateAndReset for proper cleanup - Ensure third-party HTTP libraries like fastshot are properly mocked - Fix create_smart_address HTTP mocking in CI environment
1 participant
Description
This PR fixes a critical bug in provider profile updates where the dashboard was returning "Profile updated successfully" instead of proper error handling when ProviderOrderToken creation failed due to unique constraint violations.
Root Cause:
The original code had a transaction boundary issue where ProviderOrderToken operations were inside individual transactions, but provider profile updates happened outside the transaction. When ProviderOrderToken creation failed and rolled back, the provider profile update still succeeded, causing silent failures.
Key Changes:
Schema Fix:
networkfield alongside provider, token, and currencyTwo-Phase Provider Profile Updates:
Breaking Changes:
References
Testing
Manual Testing Steps:
Environment:
Language: Go 1.21+
Platform: Linux/macOS
Database: PostgreSQL with Ent ORM
This change adds test coverage for new/changed/fixed functionality
Checklist
mainBy submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Chores