feat: add provider fee tracking to payment orders#733
Open
Dprof-in-tech wants to merge 1 commit intomainfrom
Open
feat: add provider fee tracking to payment orders#733Dprof-in-tech wants to merge 1 commit intomainfrom
Dprof-in-tech wants to merge 1 commit intomainfrom
Conversation
Dprof-in-tech
requested review from
5ran6,
chibie and
onahprosper
as code owners
Contributor
📝 WalkthroughWalkthroughThis PR adds provider fee tracking to payment orders by introducing a new Changes
Sequence Diagram(s)sequenceDiagram
participant Blockchain as Blockchain Network
participant Indexer as EVM Indexer
participant Decoder as Event Decoder
participant Service as Fee Service
participant Database as Payment Orders DB
Blockchain->>Indexer: Emit LocalTransferFeeSplit Event (orderId, providerAmount, ...)
Indexer->>Decoder: Decode RPC Log via DecodeLocalTransferFeeSplitEvent
Decoder-->>Indexer: Extract orderId, amounts, return map
Indexer->>Service: ProcessLocalTransferFeeSplitEvents(network, events)
Service->>Service: For each event: UpdateProviderFee(network, event)
Service->>Database: Query PaymentOrder by GatewayID & Network
Service->>Database: Set provider_fee = event.ProviderAmount
Database-->>Service: Update result (error or success)
Service-->>Indexer: Log error if update fails, continue
Indexer->>Database: Store LocalTransferFeeSplit event count
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
controllers/sender/sender.go (1)
2937-2969: Don’t derive historical provider-fee totals from today’s FX rates.This path divides
paymentOrder.ProviderFeeby the currentMarketBuyRate/MarketSellRate, soTotalProviderFeeswill drift as rates move. If this metric is meant for reconciliation/reporting, prefer an order-time USD amount or a rate captured at settlement.Also applies to: 2984-2988
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/sender/sender.go` around lines 2937 - 2969, The code is incorrectly converting historical paymentOrder.ProviderFee using current MarketBuyRate/MarketSellRate (via fiatCurrency.MarketBuyRate/MarketSellRate), causing TotalProviderFees to drift; change the logic to use the order-recorded USD amount or the FX rate captured at order/settlement time (e.g., paymentOrder.ProviderFeeUSD or paymentOrder.SettlementRate or paymentOrder.FxRate) when computing localStablecoinProviderFee instead of dividing by fiatCurrency.MarketBuyRate/MarketSellRate; if such a stored field is missing, fall back to skipping conversion and log a warning so historical totals remain stable (update references in the loop handling paymentOrder.ProviderFee and the similar block around the later lines 2984-2988).utils/rpc_events.go (2)
235-237: Tighten fixed-size ABI payload validation to exact length.For this event ABI, payload should be exactly 96 bytes (3 × uint256). Using
len(log.Data) < 96is permissive and can mask malformed payloads.Proposed patch
- if len(log.Data) < 96 { - return nil, fmt.Errorf("invalid LocalTransferFeeSplit event data: too short") + if len(log.Data) != 96 { + return nil, fmt.Errorf("invalid LocalTransferFeeSplit event data: expected 96 bytes, got %d", len(log.Data)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/rpc_events.go` around lines 235 - 237, The length check for the LocalTransferFeeSplit event is too permissive; replace the current conditional that checks len(log.Data) < 96 with a strict equality check (len(log.Data) != 96) and update the error message to state "invalid LocalTransferFeeSplit event data: expected 96 bytes" so malformed payloads of incorrect length are rejected; locate the validation around the LocalTransferFeeSplit handling where log.Data is inspected and adjust the condition and message accordingly.
22-25: Add a verification test for event signature hashes to catch ABI drift.The hardcoded event signature hashes are currently correct, but there's no test to verify they remain consistent with their canonical signatures. If the contract ABI changes, these constants could silently drift and break indexing. Add a test that computes keccak256 of the event signatures and asserts they match the constants.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/rpc_events.go` around lines 22 - 25, Add a unit test that computes the keccak256 hash of the canonical event signature strings and asserts they match the existing constants SettleInEventSignature and LocalTransferFeeSplitEventSignature in utils/rpc_events.go; specifically, derive the hash from the exact signature text (e.g. "SettleIn(bytes32...)" and "LocalTransferFeeSplit(bytes32 indexed orderId, uint256 senderAmount, uint256 providerAmount, uint256 aggregatorAmount)"), compare to the constants and fail the test if they differ so ABI drift is detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ent/schema/paymentorder.go`:
- Around line 45-47: The schema currently uses
field.Float("provider_fee").GoType(decimal.Decimal{}).DefaultFunc(...), which
sets a zero value and conflates "unknown" with genuine zero; remove the
DefaultFunc and make the column nullable/optional instead (e.g., use
field.Float("provider_fee").GoType((*decimal.Decimal)(nil)).Optional().Nillable()
or the project-preferred Optional/Nillable pattern) so new rows and migrated
rows can represent "fee not yet set" as NULL; update any code that reads/writes
provider_fee to handle a nil/*decimal.Decimal rather than assuming a non-nil
decimal.
In `@services/common/order.go`:
- Around line 61-72: The current code uses db.Client.PaymentOrder.Update() with
paymentorder.GatewayIDEQ(...) and related predicates which performs a bulk
update and ignores the affected-row count; instead, first fetch a single
PaymentOrder using
db.Client.PaymentOrder.Query().Where(paymentorder.GatewayIDEQ(gatewayID),
paymentorder.HasTokenWith(tokenent.HasNetworkWith(networkent.IdentifierEQ(network.Identifier)))).Only(ctx)
to resolve exactly one order (handle not-found and not-singular errors), then
call
db.Client.PaymentOrder.UpdateOneID(order.ID).SetProviderFee(event.ProviderAmount).Save(ctx)
and check/return/save any errors from both the query and the update (do not
treat “no matching order” as success).
In `@services/indexer/evm.go`:
- Around line 854-892: The code currently takes the raw uint256 string parsed
into providerAmount and stores it into LocalTransferFeeSplitEvent.ProviderAmount
(which later flows into UpdateProviderFee) without converting from base/minor
units to token decimals; change the creation of providerAmount so it is
normalized to human/token decimals before assigning to feeSplitEvent (use the
existing token-decimals lookup or a helper like
utils.NormalizeTokenAmount/WeiToDecimal with the appropriate token decimals),
i.e., parse providerAmountStr as you do now, then scale it by 10^-decimals (or
call the project normalization helper) and use that normalized decimal for
LocalTransferFeeSplitEvent.ProviderAmount so UpdateProviderFee receives the
correct decimal amount.
---
Nitpick comments:
In `@controllers/sender/sender.go`:
- Around line 2937-2969: The code is incorrectly converting historical
paymentOrder.ProviderFee using current MarketBuyRate/MarketSellRate (via
fiatCurrency.MarketBuyRate/MarketSellRate), causing TotalProviderFees to drift;
change the logic to use the order-recorded USD amount or the FX rate captured at
order/settlement time (e.g., paymentOrder.ProviderFeeUSD or
paymentOrder.SettlementRate or paymentOrder.FxRate) when computing
localStablecoinProviderFee instead of dividing by
fiatCurrency.MarketBuyRate/MarketSellRate; if such a stored field is missing,
fall back to skipping conversion and log a warning so historical totals remain
stable (update references in the loop handling paymentOrder.ProviderFee and the
similar block around the later lines 2984-2988).
In `@utils/rpc_events.go`:
- Around line 235-237: The length check for the LocalTransferFeeSplit event is
too permissive; replace the current conditional that checks len(log.Data) < 96
with a strict equality check (len(log.Data) != 96) and update the error message
to state "invalid LocalTransferFeeSplit event data: expected 96 bytes" so
malformed payloads of incorrect length are rejected; locate the validation
around the LocalTransferFeeSplit handling where log.Data is inspected and adjust
the condition and message accordingly.
- Around line 22-25: Add a unit test that computes the keccak256 hash of the
canonical event signature strings and asserts they match the existing constants
SettleInEventSignature and LocalTransferFeeSplitEventSignature in
utils/rpc_events.go; specifically, derive the hash from the exact signature text
(e.g. "SettleIn(bytes32...)" and "LocalTransferFeeSplit(bytes32 indexed orderId,
uint256 senderAmount, uint256 providerAmount, uint256 aggregatorAmount)"),
compare to the constants and fail the test if they differ so ABI drift is
detected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5322c92-006e-4763-9407-41e972240248
⛔ Files ignored due to path filters (1)
ent/migrate/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
controllers/sender/sender.goent/migrate/migrations/20260319165304_add_provider_fee.sqlent/migrate/schema.goent/mutation.goent/paymentorder.goent/paymentorder/paymentorder.goent/paymentorder/where.goent/paymentorder_create.goent/paymentorder_update.goent/runtime/runtime.goent/schema/paymentorder.goservices/common/indexer.goservices/common/order.goservices/indexer/evm.gotypes/types.goutils/rpc_events.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pull request introduces support for tracking and aggregating provider fees in payment orders. The main changes add a new
provider_feefield to the database and update relevant application logic to handle this new field, including mutation handling, statistics calculation, and schema management.Provider Fee Field Addition and Schema Migration:
provider_feecolumn to thepayment_orderstable via a migration (20260319165304_add_provider_fee.sql) and updated migration checksums inatlas.sumand schema definitions inschema.go. [1] [2] [3] [4]Application Logic Updates:
PaymentOrderstruct and related methods to include the newprovider_feefield, ensuring correct assignment, scanning, and string representation. [1] [2] [3] [4]PaymentOrderMutationstruct and its methods to support setting, adding, resetting, and querying theprovider_feefield, mirroring existing fee fields. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]Statistics Calculation Enhancements:
Statsmethod inSenderControllerto aggregate and convert provider fees alongside sender fees and total volume, and include provider fee totals in the statistics response. [1] [2] [3] [4] [5]Schema and Field Constants Update:
provider_feeto payment order field constants, columns, and default value declarations to ensure consistency across the codebase. [1] [2] [3]Schema Index and Foreign Key Adjustments:
schema.goto accommodate the newprovider_feefield, ensuring migration and schema integrity. [1] [2]References
closes #597
Testing
Checklist
mainBy submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
Release Notes