feat: Enhance event handling and parsing in Starknet client #627
Conversation
- Added decimal parsing for amounts, protocol fees, rates, settle percentages, rebate percentages, and fees in handleOrderCreated, handleOrderSettled, and handleOrderRefunded methods. - Replaced custom byte array parsing with a utility function in utils.go. - Normalized Starknet addresses in event data for better consistency. - Improved error handling for parsing operations. - Refactored Voyager event transformation to include detailed error handling and support for new data formats. - Updated Voyager API calls to remove unnecessary logging for cleaner output.
sundayonah
requested review from
5ran6,
chibie and
onahprosper
as code owners
WalkthroughAdds Starknet-aware pathParam parsing in the controller, converts Starknet indexer flows from per-transaction to batch RPC-receipt processing, normalizes/parses event fields (addresses, decimals, Cairo ByteArray), exposes a Voyager RPC fallback, and adds ByteArray/decimal parsing utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant VoyagerService
participant IndexerStarknet
participant DB
Client->>Controller: HTTP /index/:pathParam or from_block/to_block
Controller->>Controller: resolve network (Starknet vs other)
Controller->>VoyagerService: GetEventsByTransactionHashRPC(txHash)
VoyagerService-->>Controller: []rpcEvents (batch receipts)
Controller->>IndexerStarknet: index...ByTransaction(events batch)
IndexerStarknet->>IndexerStarknet: normalize addresses, parse decimals, ParseByteArray
IndexerStarknet->>DB: persist aggregated event results
DB-->>IndexerStarknet: ack
IndexerStarknet-->>Controller: EventCounts
Controller-->>Client: HTTP response (counts/status)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/voyager.go (1)
1105-1247:TransformVoyagerEventToRPCFormatemitstopicbut indexer readstopicsThe new
TransformVoyagerEventToRPCFormatcorrectly:
- Builds
indexed_params/non_indexed_paramsmaps forOrderCreated,OrderSettled, andOrderRefunded.- Parses numeric fields (
amount,protocol_fee,rate,settle_percent,rebate_percent,fee) intodecimal.Decimal.- Decodes
message_hashByteArray viau.ParseByteArrayFromJSON.- Normalizes
addressviaNormalizeStarknetAddress.However, it writes the selector under the key:
"topic": keys[0],while
indexer/starknet.go’sindexGatewayByTransactionreads:eventSignature, ok := eventMap["topics"].(string)so any gateway events coming from Voyager+this transformer will have no
"topics"field and will be skipped by the indexer. Only RPC‑originated events (which still use"topics") will be processed.To fix this, either:
- Change the transformer to emit
"topics"(string) to mirror the RPC client shape:- "topic": keys[0], + "topics": fmt.Sprintf("%v", keys[0]),or
- Update the indexer to accept both, e.g.:
sig, _ := eventMap["topics"].(string) if sig == "" { if v, ok := eventMap["topic"].(string); ok { sig = v } }Using the same key name as the RPC path is simpler and less error‑prone.
services/indexer/starknet.go (1)
274-303: Gateway indexing ignores Voyager-transformed events due totopicsvstopicmismatch
indexGatewayByTransactioncurrently does:eventSignature, ok := eventMap["topics"].(string) if !ok { logger.Errorf("Missing or invalid 'topics' field in gateway event") continue }and then switches on
eventSignatureagainst the Starknet selectors.When
IndexGatewayuses Voyager as the data source (GetContractEvents+TransformVoyagerEventToRPCFormat), the transformed events carry the selector under"topic", not"topics". As a result, all Voyager‑sourced gateway events are skipped here, and only pure RPC‑sourced events are processed.Once you align
TransformVoyagerEventToRPCFormatto emit"topics"(or update this code to fall back to"topic"), this will work for both sources. Until then, gateway indexing in Voyager‑success cases will silently do nothing.This is coupled with the
topic/topicsissue described inservices/voyager.go.Also applies to: 315-363, 377-399, 431-462, 465-521
🧹 Nitpick comments (3)
utils/utils.go (2)
903-965: Redis bucket scanning mixes current and_prevqueues without explicit precedenceThe updated
getProviderRateFromRedis,validateBucketRate,parseBucketKey, andfindSuitableProviderRatelook structurally sound and add useful validation and logging. However:
Scanuses patternbucket_<currency>_*_*andparseBucketKeyaccepts 4‑ or 5‑part keys, so both current (bucket_NGN_1_500000) and_prev(bucket_NGN_1_500000_prev) buckets will be treated identically.- Because
validateBucketRateiterates keys in arbitrary SCAN order and stops on the firstProviderRateResult.Found == true, a_prevbucket can win over the “current” bucket instead of being used strictly as a fallback when current has no suitable provider.If the intent from the linked issue is “_prev only as fallback when current queue has no match”, consider:
- Splitting keys into “current” and
_prevgroups via the 5th segment and trying all currents first, only then_prevbuckets.- Or scanning twice (once with a pattern that excludes
_prev, then with one that only includes_prev).Also note that
findSuitableProviderRateperforms multiple Ent queries per provider; if this path becomes hot, cachingProviderOrderToken/ProviderCurrenciesper provider+token+currency (as hinted by the TODO) would help.Also applies to: 968-1047
1411-1458: Cairo ByteArray and string→decimal helpers look correct; consider stricter validation
ParseByteArraymatches Cairo’s ByteArray layout (num full 31‑byte chunks, then pending word + length) and safely guards against out‑of‑bounds access whenlen(data) < numFullChunks+3.ParseByteArrayFromJSONcorrectly reconstructs the felt slice from{data.value, pending_word.value, pending_word_len.value}and delegates toParseByteArray. Right now, invalid full‑chunk entries (HexToFelterror) are silently skipped, which can causenum_full_chunksto overcount and return an empty string. If malformed Voyager payloads are a concern, failing fast (return "", err) instead ofcontinuewould be safer and easier to debug.ParseStringAsDecimalsandparseHexStringhandle both0x/0Xhex and plain decimal strings and usedecimal.NewFromBigIntfor hex, which is appropriate for large on‑chain values.Overall the utilities look good and align with how Starknet events are consumed elsewhere; the only suggestion is to treat inconsistent ByteArray JSON as an error instead of degrading to
"".Also applies to: 1460-1529, 1531-1568
services/starknet/client.go (1)
676-709: OrderCreated event shaping aligns with indexer expectationsThe refactor to:
- Convert
amount,protocolFee, andrateintodecimal.Decimalviau.ParseStringAsDecimals.- Decode
message_hashusingu.ParseByteArray.- Normalize
address,sender, andtokenviacryptoUtils.NormalizeStarknetAddress.is consistent with how
indexer/starknet.goreadsdecoded.indexed_params.amount(asdecimal.Decimal) and non‑indexed params, and it keeps event shape compatible between RPC and Voyager paths. The extraParseStringAsDecimals(bigInt.String())hop is slightly redundant but harmless.Also applies to: 714-728
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
controllers/index.go(1 hunks)services/indexer/starknet.go(18 hunks)services/starknet/client.go(9 hunks)services/voyager.go(7 hunks)utils/utils.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
controllers/index.go (2)
ent/network/where.go (1)
Identifier(75-77)utils/http.go (1)
APIResponse(23-29)
services/starknet/client.go (2)
utils/utils.go (2)
ParseStringAsDecimals(1531-1548)ParseByteArray(1413-1458)utils/crypto/crypto.go (1)
NormalizeStarknetAddress(312-324)
services/indexer/starknet.go (7)
ent/token/where.go (1)
ContractAddress(74-76)services/voyager.go (2)
TransformVoyagerTransferToRPCFormat(1044-1102)TransformVoyagerEventToRPCFormat(1105-1247)ent/network/where.go (1)
GatewayContractAddress(85-87)utils/crypto/crypto.go (1)
NormalizeStarknetAddress(312-324)types/types.go (2)
EventCounts(789-794)OrderSettledEvent(84-92)utils/rpc_events.go (1)
OrderSettledStarknetSelector(18-18)services/common/indexer.go (1)
ProcessSettledOrders(119-174)
⏰ 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 (4)
services/starknet/client.go (1)
848-866: Transfer event address normalization is a nice consistency improvementNormalizing:
address(FromAddress),from, andtovia
cryptoUtils.NormalizeStarknetAddressbrings RPC transfer events in line with the normalized Starknet address format you use elsewhere (Voyager transforms, indexer). This should reduce downstream comparison issues without changing semantics.Also applies to: 855-867
services/voyager.go (2)
1045-1077: Voyager transfer → RPC transform is sound and defensively codedThe updated
TransformVoyagerTransferToRPCFormat:
- Validates
tokenDecimals > 0and non‑emptytransferValue.- Parses
transferValueasdecimal.Decimaland converts it to subunits viau.ToSubunit(decimalValue, int8(tokenDecimals)).- Emits the subunit value as a string in
decoded.non_indexed_params.valueand setstopictou.TransferStarknetSelector.This matches how the Starknet indexer later expects a string subunit amount and keeps the RPC‑shaped transfer events consistent with the native Starknet client path. Returning
nilon invalid inputs and letting the caller skip those transfers is a reasonable degradation strategy.Also applies to: 1083-1096
1627-1665: ExportedGetEventsByTransactionHashRPCis wired correctly into fallbacksThe refactor to:
- Introduce
GetEventsByTransactionHashRPC(ctx, txHash)and- Use it as the RPC fallback in both
GetEventsByTransactionHashImmediateand the queuedGetEventsByTransactionHashis consistent with how other RPC fallbacks in this service are structured. It centralizes the Starknet client call and keeps the call sites cleaner without changing behavior.
Also applies to: 1723-1726, 1763-1771
services/indexer/starknet.go (1)
52-63: Receive-address indexing flow is coherent with new RPC/Voyager event shapes
- For a specific
txHash, you now pull events viaGetEventsByTransactionHashRPCand accumulate them intotransactionHashReceipt.- For range/last‑N mode, you fetch transfers via
GetAddressTokenTransfers, convert each withTransformVoyagerTransferToRPCFormat, and feed the RPC‑shaped events intoprocessReceiveAddressByTransactionEvents.processReceiveAddressByTransactionEventsthen:
- Validates
decoded.non_indexed_params.{from,to,value}are strings,- Normalizes addresses when comparing with
userAccountAddressand the gateway contract,- Converts
valuefrom subunits using token decimals before passing tocommon.ProcessTransfers.This end‑to‑end path looks consistent and robust; logging will also be much easier to reason about now.
Also applies to: 75-86, 100-122, 133-142, 186-203, 250-259
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
services/voyager.go (1)
1126-1231: Consider adding a default case for unknown event types.The switch statement handles three specific Starknet event selectors but lacks a default case. If an unrecognized topic is encountered, the function will silently return an event with empty
indexed_paramsandnon_indexed_params. Consider whether this is the intended behavior or if unknown event types should be logged or return an error.Optional: Add default case with logging
case u.OrderRefundedStarknetSelector: // ... existing code ... + default: + logger.WithFields(logger.Fields{ + "Topic": topics, + "TransactionHash": transactionHash, + }).Warnf("Unrecognized event topic, returning empty params") }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/voyager.go(8 hunks)
⏰ 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 (5)
services/voyager.go (5)
709-710: LGTM: Expanded default lookback window.The default timestamp window has been expanded from 4 to 7 days, which provides a wider range for transfer queries when block numbers are not specified.
1054-1076: LGTM: Robust validation for transfer value parsing.The added validation ensures that
tokenDecimalsis positive andtransferValueis non-empty before attempting decimal parsing and conversion to subunits. This prevents downstream errors and improves data quality.
1083-1095: LGTM: Consistent use of converted subunit values.The changes correctly use the parsed and converted
rawValueDecimalsvalue instead of the raw string, and add thetopicfield for event type identification. This aligns with the standardized RPC format.
1667-1668: LGTM: Exported RPC fallback function.Making
GetEventsByTransactionHashRPCa public method allows external callers to use the RPC fallback path directly. All internal call sites have been updated consistently.
1105-1246: Note: PR objectives mismatch.The linked issue #407 discusses saving previous provider rates in priority queues to reduce order failures, but the changes in this file focus on Voyager event transformation and parsing. The code changes appear to be well-implemented for their purpose, but there's a disconnect between the PR's linked issue and the actual changes being made.
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 (2)
services/indexer/starknet.go (1)
223-258: Starknet indexer assumesblock_numberisint64but emitters provide other typesThroughout the Starknet indexer you assert:
blockNumber, ok := eventMap["block_number"].(int64) ... if !ok { /* skip */ }in:
processReceiveAddressByTransactionEvents(Line 223),indexGatewayByTransaction(Line 301),indexProviderAddressByTransaction(Line 684),but:
- The Starknet RPC client currently stores
block_numberas auint64(emittedEvent.BlockNumber).- Voyager transforms for transfers/events presently don’t normalize to
int64and may leaveblock_numberas 0.Net effect: every Starknet event coming from the RPC path, and many from Voyager, will be skipped here because the type assertion fails.
Once
client.goand Voyager transforms are updated to emitint64(or at least a consistent numeric type), this code will work. Until then, either:
Accept multiple numeric forms and normalize locally:
var blockNumber int64 switch v := eventMap["block_number"].(type) { case int64: blockNumber = v case uint64: blockNumber = int64(v) case float64: blockNumber = int64(v) default: logger.Errorf("Unexpected block_number type %T", v) continue }Or, better, fix all emitters (RPC client and Voyager transformers) to set
block_numberasint64and keep the assertions strict here.Right now this mismatch is a functional blocker for Starknet indexing.
Also applies to: 301-307, 684-688
controllers/index.go (1)
2233-2247: Avoid EVM-only type assertion inIndexTransactionwhen network is StarknetYou’ve correctly introduced Starknet support:
} else if strings.HasPrefix(network.Identifier, "starknet") { indexerInstance, indexerErr = indexer.NewIndexerStarknet()but later, when handling an address that’s found as a receive address, you still do:
counts, err := indexerInstance.(*indexer.IndexerEVM). IndexReceiveAddressWithBypass(ctx, token, address, fromBlock, toBlock, txHash, true)This downcast assumes
indexerInstanceis always*IndexerEVM. For Starknet networks, it will actually be*IndexerStarknet, so this cast will panic at runtime as soon as a Starknet receive address is indexed.You need to branch by network type here as well. For example:
Sketch of network-aware receive-address indexing
- counts, err := indexerInstance.(*indexer.IndexerEVM).IndexReceiveAddressWithBypass( - ctx, token, address, fromBlock, toBlock, txHash, true) + switch idx := indexerInstance.(type) { + case *indexer.IndexerEVM: + counts, err := idx.IndexReceiveAddressWithBypass( + ctx, token, address, fromBlock, toBlock, txHash, true) + case *indexer.IndexerStarknet: + counts, err := idx.IndexReceiveAddress( + ctx, token, address, fromBlock, toBlock, txHash) + default: + err = fmt.Errorf("unsupported indexer type %T for receive address", idx) + }(adjust signatures to match your Starknet indexer API.)
Without this change, Starknet address-based indexing via
IndexTransactionis unsafe.Also applies to: 2407-2476
♻️ Duplicate comments (2)
services/starknet/client.go (1)
712-722: Unifyblock_numbertype across emitters and indexer (currently uint64 vs int64)In all handlers you emit:
"block_number": emittedEvent.BlockNumber, // uint64but the Starknet indexer now does:
blockNumber, ok := eventMap["block_number"].(int64)in both
processReceiveAddressByTransactionEvents,indexGatewayByTransaction, andindexProviderAddressByTransaction. That type mismatch means the assertions will always fail for RPC‑shaped events, so Starknet transfers and gateway/provider events from the RPC path will be silently skipped.To fix this, standardize
block_numberon a single type (int64 is already assumed by the indexer). For example:Proposed fix: cast to
int64in all handlers- event := map[string]interface{}{ - "block_number": emittedEvent.BlockNumber, + event := map[string]interface{}{ + "block_number": int64(emittedEvent.BlockNumber),Apply the same change in
handleOrderCreated,handleOrderSettled,handleOrderRefunded, andhandleTransfer.Also applies to: 775-789, 822-832, 855-867
services/voyager.go (1)
1105-1174: Align Voyager event transform with RPC/indexer schema and avoid unsafe assertions
TransformVoyagerEventToRPCFormathas several schema and safety problems that will break Starknet gateway indexing:
- Block number typing from Voyager
You currently do:
blockNumber, _ := event["blockNumber"].(int64)but elsewhere in this file you treat Voyager
blockNumbervalues asfloat64(block["blockNumber"].(float64)inmakeVoyagerBlocksAPICall). This assertion will almost always fail, leavingblockNumber == 0in the output rpcEvent, while the indexer expects a meaningfulint64block number. Use the same pattern as for transfers: acceptfloat64and convert toint64.
topicvstopicsfield mismatch with indexerThe output map sets:
"topic": keys[0],but
indexGatewayByTransactionreads:eventSignature, ok := eventMap["topics"].(string)So Voyager‑transformed gateway events will never be recognized by the indexer. You should either:
- Emit
"topics"as a string (using the already‑derivedtopicsvariable), or- Update the indexer to consume
"topic"consistently.Example fix on the transform side:
- "topic": keys[0], + "topics": topics,
- Inconsistent amount typing vs RPC path
For
OrderCreatedyou set:case "amount": orderAmount, err := u.ParseStringAsDecimals(keyValue.(string)) ... indexedParams[keyName] = orderAmount.String()Whereas the RPC Starknet client emits
amountasdecimal.Decimal(and the indexer assertsindexedParams["amount"].(decimal.Decimal)). This means Voyager‑originatedOrderCreatedevents will be silently skipped byindexGatewayByTransaction.Either:
- Change this to store the
decimal.Decimaldirectly (no.String()), or- Teach the indexer to accept both
decimal.Decimalandstring(and parse the latter).
- Unchecked type assertions can still panic (similar to a prior review)
Patterns like:
orderAmount, err := u.ParseStringAsDecimals(keyValue.(string)) ... valueDecimals, err := u.ParseStringAsDecimals(dataValue.(string)) ... dataValue, _ := dataMap["value"].(string)will panic if Voyager returns a non‑string (e.g.,
nil, numeric, or nested map). The previous review already flagged this class of issues.Consider safe assertions with descriptive errors, e.g.:
valStr, ok := dataValue.(string) if !ok { return nil, fmt.Errorf("expected string for %s, got %T", dataName, dataValue) }and likewise for all
...(string)assertions in this function.Together, these issues will cause Voyager‑based Starknet event indexing either to skip events or panic. Fixing them will make Voyager and RPC paths interoperable with the updated Starknet indexer.
Also applies to: 1185-1232, 1237-1247
🧹 Nitpick comments (2)
services/starknet/client.go (1)
751-773: Simplifysettle_percent/rebate_percentparsing and dropextractUint64AsString
handleOrderSettledcurrently converts:settlePercent := emittedEvent.Data[1].BigInt(...).Uint64() settlePercentStr, ok := extractUint64AsString(settlePercent) ... settledPercentDecimals, err := u.ParseStringAsDecimals(settlePercentStr)and
extractUint64AsStringtakesinterface{}even though all callers passuint64.You can simplify and remove the helper entirely:
Proposed simplification
- settlePercent := emittedEvent.Data[1].BigInt(big.NewInt(0)).Uint64() - rebatePercent := emittedEvent.Data[2].BigInt(big.NewInt(0)).Uint64() - - settlePercentStr, ok := extractUint64AsString(settlePercent) - if !ok { - return nil, fmt.Errorf("failed to extract settle_percent as uint64") - } - rebatePercentStr, ok := extractUint64AsString(rebatePercent) - if !ok { - return nil, fmt.Errorf("failed to extract rebate_percent as uint64") - } - - settledPercentDecimals, err := u.ParseStringAsDecimals(settlePercentStr) + settlePercent := emittedEvent.Data[1].BigInt(big.NewInt(0)).Uint64() + rebatePercent := emittedEvent.Data[2].BigInt(big.NewInt(0)).Uint64() + + settledPercentDecimals, err := u.ParseStringAsDecimals(fmt.Sprintf("%d", settlePercent)) if err != nil { return nil, fmt.Errorf("failed to parse settle_percent as decimals: %w", err) } - rebatePercentDecimals, err := u.ParseStringAsDecimals(rebatePercentStr) + rebatePercentDecimals, err := u.ParseStringAsDecimals(fmt.Sprintf("%d", rebatePercent)) if err != nil { return nil, fmt.Errorf("failed to parse rebate_percent as decimals: %w", err) }and then delete
extractUint64AsStringentirely.This keeps the code straightforward and avoids an unnecessary
interface{}helper.Also applies to: 875-880
controllers/index.go (1)
2127-2134: Clarify whether range-only indexing (without pathParam) is intentionally disallowedThe new validation:
pathParam := ctx.Param("tx_hash_or_address") if pathParam == "" || !strings.HasPrefix(pathParam, "0x") { u.APIResponse(..., "Invalid path parameter. Must be a valid transaction hash or address") return }means callers can no longer index purely by
from_block/to_blockrange; they must always provide a non-empty,0x-prefixed path parameter even if they’re only interested in block ranges. This tightens behavior compared to the earlier version and to the subsequent check:if txHash == "" && address == "" && (fromBlockStr == "" || toBlockStr == "") { // ... }If that stricter behavior is intentional, consider updating API docs to make it explicit. If you still want to support range-only indexing, change the validation to only enforce the
0xprefix whenpathParam != ""and rely on the later “at least one indexing method” check.Also applies to: 2226-2230
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
controllers/index.go(3 hunks)services/indexer/starknet.go(18 hunks)services/starknet/client.go(9 hunks)services/voyager.go(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
services/starknet/client.go (2)
utils/utils.go (2)
ParseStringAsDecimals(1531-1548)ParseByteArray(1413-1458)utils/crypto/crypto.go (1)
NormalizeStarknetAddress(312-324)
services/voyager.go (4)
utils/logger/logger.go (3)
WithFields(76-109)Fields(73-73)Errorf(141-146)utils/utils.go (3)
ToSubunit(44-56)ParseStringAsDecimals(1531-1548)ParseByteArrayFromJSON(1460-1529)utils/crypto/crypto.go (1)
NormalizeStarknetAddress(312-324)utils/rpc_events.go (3)
OrderCreatedStarknetSelector(17-17)OrderSettledStarknetSelector(18-18)OrderRefundedStarknetSelector(19-19)
⏰ 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)
services/indexer/starknet.go (3)
132-143: Receive-address transfer processing looks good but relies on consistent event schemaThe refactor to batch process
transferEventsviaprocessReceiveAddressByTransactionEventswith:
- Normalized Starknet addresses (
NormalizeStarknetAddressonfrom/toand user address),- Robust checks for missing/empty decoded and non_indexed_params,
- Decimal parsing of
valueStrand rescaling by token decimals,is solid and aligns well with the new Voyager→RPC transfer transform.
Once
block_numbertyping is unified (per other comments), this path should correctly handle both RPC receipts (viaGetEventsByTransactionHashRPC) and Voyager transfer events transformed throughTransformVoyagerTransferToRPCFormat.Also applies to: 186-268
273-347: Gateway event parsing assumes decimal+schema alignment with both RPC and Voyager transformsThe new
indexGatewayByTransactionlogic is directionally correct: it batches all gateway events and constructs typedOrderCreatedEvent,OrderSettledEvent, andOrderRefundedEventfrom decoded params.However, it assumes:
indexed_params["amount"],["sender"], and["token"]exist and:
amountisdecimal.Decimal,sender/tokenare strings.non_indexed_params["protocol_fee"],["rate"],["settle_percent"],["rebate_percent"], and["fee"]are alldecimal.Decimal.Given:
- The RPC Starknet client now emits these values as
decimal.Decimal(good).TransformVoyagerEventToRPCFormatcurrently storesamountas a string (notdecimal.Decimal) and emits"topic"instead of"topics",Voyager‑originated events will currently be dropped here (type assertions fail or
topicsmissing). Once the transformer is updated as suggested inservices/voyager.go(emittopicsas string and usedecimal.Decimalforamount), this batching logic should work for both sources.No change needed here, but it’s tightly coupled to those upstream fixes; keep that dependency in mind when testing.
Also applies to: 375-399, 429-459
641-751: Provider settlement indexing is consistent with new decimal schema but blocked byblock_numbertype
indexProviderAddressByTransactioncorrectly:
- Filters only
OrderSettledevents by selector/topic.- Narrows to events whose
liquidity_providermatches the normalized provider address.- Expects
settle_percentandrebate_percentasdecimal.Decimal, matching the updated RPC and Voyager transforms.- Builds
OrderSettledEventvalues and delegates tocommon.ProcessSettledOrders.The only hard blocker here is again the
block_numberassertion asint64. Once emitters are fixed to useint64(or you relax the assertion as suggested earlier), this method should behave as intended.
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)
services/starknet/client.go (1)
840-853: Bounds check missing foremittedEvent.Dataaccess; potential panic.The comment at lines 841-842 states data is in keys for Transfer events, but lines 848-853 access
emittedEvent.Data[0]throughData[3]without verifying length. IfDatahas fewer than 4 elements, this will panic.🔎 Proposed fix with bounds check
func (c *Client) handleTransfer(emittedEvent rpc.EmittedEvent) (map[string]interface{}, error) { // Keys: [event_selector, from, to, value_low, value_high] - // Data: [] (empty - all data is in keys for Transfer event on Starknet) + // Data: [from, to, value_low, value_high] for some token contracts if len(emittedEvent.Keys) == 0 { return nil, fmt.Errorf("invalid Transfer event: insufficient keys") } + if len(emittedEvent.Data) < 4 { + return nil, fmt.Errorf("invalid Transfer event: insufficient data (expected 4, got %d)", len(emittedEvent.Data)) + } + from := emittedEvent.Data[0] to := emittedEvent.Data[1]
♻️ Duplicate comments (4)
services/voyager.go (3)
1053-1060: ValidatetokenDecimalstype before comparison.Line 1052 asserts
tokenDecimalsasint, but JSON unmarshaling producesfloat64for numbers. This assertion will silently yield0, triggering the warning at lines 1054-1060 for valid transfers.This duplicates the past review comment about numeric type handling. The fix should handle multiple numeric types:
- tokenDecimals, _ := transfer["tokenDecimals"].(int) + var tokenDecimals int + switch v := transfer["tokenDecimals"].(type) { + case float64: + tokenDecimals = int(v) + case int: + tokenDecimals = v + case int64: + tokenDecimals = int(v) + }
1130-1183: Unchecked type assertions may panic on unexpected Voyager data.Multiple unchecked type assertions remain in the
OrderCreatedStarknetSelectorcase:
- Line 1139:
keyValue.(string)- Line 1158:
dataValue.(string)If Voyager returns unexpected types, these will panic.
This was flagged in a past review. Apply safe assertions consistently:
case "amount": - orderAmount, err := u.ParseStringAsDecimals(keyValue.(string)) + keyValueStr, ok := keyValue.(string) + if !ok { + return nil, fmt.Errorf("expected string for amount, got %T", keyValue) + } + orderAmount, err := u.ParseStringAsDecimals(keyValueStr)
1222-1232: Unchecked type assertion inOrderRefundedStarknetSelectorcase.Line 1225 asserts
dataValueasstringwithout checking. If the fee value comes as a different type (e.g., a number), this will panic.- dataValue, _ := dataMap["value"].(string) - feeValue, err := u.ParseStringAsDecimals(dataValue) + dataValue, ok := dataMap["value"].(string) + if !ok { + return nil, fmt.Errorf("expected string for fee value, got %T", dataMap["value"]) + } + feeValue, err := u.ParseStringAsDecimals(dataValue)services/indexer/starknet.go (1)
322-325: Type mismatch:amountis string from Voyager but expected asdecimal.Decimal.Line 322-323 expects
indexedParams["amount"].(decimal.Decimal), butTransformVoyagerEventToRPCFormatin voyager.go (line 1143) sets it asorderAmount.String()— a string, notdecimal.Decimal.This will cause all Voyager-sourced OrderCreated events to be silently skipped.
🔎 Fix in voyager.go or accept both types here
Option 1: Fix voyager.go line 1143
- indexedParams[keyName] = orderAmount.String() + indexedParams[keyName] = orderAmountOption 2: Handle both types in starknet.go
- orderAmount, ok := indexedParams["amount"].(decimal.Decimal) - if !ok { - continue - } + var orderAmount decimal.Decimal + switch v := indexedParams["amount"].(type) { + case decimal.Decimal: + orderAmount = v + case string: + var err error + orderAmount, err = decimal.NewFromString(v) + if err != nil { + continue + } + default: + continue + }
🧹 Nitpick comments (3)
services/starknet/client.go (2)
755-773: SimplifyextractUint64AsStringusage or inline conversion.
settlePercentandrebatePercentat lines 752-753 are alreadyuint64values after theBigInt().Uint64()call. TheextractUint64AsStringhelper then type-asserts them back touint64, which is redundant. Consider inlining the conversion:- settlePercentStr, ok := extractUint64AsString(settlePercent) - if !ok { - return nil, fmt.Errorf("failed to extract settle_percent as uint64") - } + settlePercentStr := fmt.Sprintf("%d", settlePercent)The current code works correctly but adds unnecessary indirection.
775-791: Inconsistent address normalization inhandleOrderSettled.
handleOrderCreated(lines 714, 719-720) andhandleTransfer(lines 858, 864-865) normalize addresses usingcryptoUtils.NormalizeStarknetAddress, buthandleOrderSettledoutputs raw addresses at lines 778 and 783 (liquidityProvider.String(),emittedEvent.FromAddress.String()). This inconsistency may cause address comparison failures downstream.🔎 Proposed fix for consistency
event := map[string]interface{}{ "block_number": float64(emittedEvent.BlockNumber), "transaction_hash": emittedEvent.TransactionHash.String(), - "address": emittedEvent.FromAddress.String(), + "address": cryptoUtils.NormalizeStarknetAddress(emittedEvent.FromAddress.String()), "topics": emittedEvent.Keys[0].String(), "decoded": map[string]interface{}{ "indexed_params": map[string]interface{}{ "order_id": orderID.String(), - "liquidity_provider": liquidityProvider.String(), + "liquidity_provider": cryptoUtils.NormalizeStarknetAddress(liquidityProvider.String()), },Apply the same pattern to
handleOrderRefundedat line 824.services/indexer/starknet.go (1)
610-623: Consider deduplicating transaction hashes before fetching receipts.The loop fetches RPC receipts for each transfer, but multiple transfers could share the same
txHash. Consider collecting unique hashes first to avoid redundant RPC calls:txHashSet := make(map[string]bool) for _, transfer := range transfers { if txHash, _ := transfer["txHash"].(string); txHash != "" { txHashSet[txHash] = true } } for txHash := range txHashSet { eventsByTx, err := s.voyagerService.GetEventsByTransactionHashRPC(ctx, txHash) // ... }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
services/indexer/starknet.go(18 hunks)services/starknet/client.go(9 hunks)services/voyager.go(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
services/voyager.go (4)
utils/logger/logger.go (5)
WithFields(76-109)Fields(73-73)Warnf(133-138)Infof(125-130)Errorf(141-146)utils/utils.go (3)
ToSubunit(44-56)ParseStringAsDecimals(1531-1548)ParseByteArrayFromJSON(1460-1529)utils/crypto/crypto.go (1)
NormalizeStarknetAddress(312-324)utils/rpc_events.go (3)
OrderCreatedStarknetSelector(17-17)OrderSettledStarknetSelector(18-18)OrderRefundedStarknetSelector(19-19)
services/indexer/starknet.go (8)
ent/token/where.go (1)
ContractAddress(74-76)services/voyager.go (2)
TransformVoyagerTransferToRPCFormat(1044-1102)TransformVoyagerEventToRPCFormat(1105-1250)ent/network/where.go (1)
GatewayContractAddress(85-87)utils/crypto/crypto.go (1)
NormalizeStarknetAddress(312-324)ent/ent.go (1)
Value(44-44)types/types.go (2)
EventCounts(789-794)OrderSettledEvent(84-92)utils/rpc_events.go (2)
OrderCreatedStarknetSelector(17-17)OrderSettledStarknetSelector(18-18)services/common/indexer.go (1)
ProcessSettledOrders(119-174)
⏰ 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 (4)
services/starknet/client.go (1)
676-709: LGTM on decimal parsing for OrderCreated event.The decimal parsing for
amount,protocol_fee, andrateis properly implemented with appropriate error handling. The switch tou.ParseByteArrayfor message_hash extraction is cleaner than the previous custom implementation.services/voyager.go (1)
1670-1689: LGTM on exportingGetEventsByTransactionHashRPC.Exporting this method enables external callers (like the Starknet indexer) to directly use the RPC fallback path for batch processing. The function properly handles nil address and topics parameters.
services/indexer/starknet.go (2)
54-62: LGTM on batch receipt collection pattern.The change from per-transaction processing to collecting receipts into
transactionHashReceiptslice enables efficient batch processing. The RPC fallback viaGetEventsByTransactionHashRPCis correctly integrated.
704-712: LGTM on decimal type handling for settlement percentages.The code correctly expects
decimal.Decimalforsettle_percentandrebate_percent, which matches the output from bothhandleOrderSettledin client.go andTransformVoyagerEventToRPCFormatin voyager.go.
Checklist
mainBy submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.