-
Notifications
You must be signed in to change notification settings - Fork 7
Add token transfer processor #195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great so far 👏
internal/db/migrations/2025-06-10.4-create_indexer_table_state_changes.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't finished reviewing the PR yet, but will leave my initial comments here.
b.base.Token = sql.NullString{String: "native"} | ||
} else if issuedAsset := asset.GetIssuedAsset(); issuedAsset != nil { | ||
b.base.Token = sql.NullString{String: fmt.Sprintf("%s:%s", issuedAsset.GetAssetCode(), issuedAsset.GetIssuer())} | ||
} | ||
} else { | ||
b.base.ContractID = sql.NullString{String: contractAddress} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 The usage of sql.NullString seems wrong here because you need to also set Valid: true
. You can use the utils.SQLNullString
helper here instead.
wallet-backend/internal/utils/sql.go
Lines 8 to 13 in 8be1927
func SQLNullString(s string) sql.NullString { | |
return sql.NullString{ | |
String: s, | |
Valid: s != "", | |
} | |
} |
// WithClaimableBalance sets the claimable balance ID | ||
func (b *StateChangeBuilder) WithClaimableBalance(balanceID string) *StateChangeBuilder { | ||
b.base.ClaimableBalanceID = sql.NullString{String: balanceID, Valid: true} | ||
return b | ||
} | ||
|
||
// WithLiquidityPool sets the liquidity pool ID | ||
func (b *StateChangeBuilder) WithLiquidityPool(poolID string) *StateChangeBuilder { | ||
b.base.LiquidityPoolID = sql.NullString{String: poolID, Valid: true} | ||
return b | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see you correctly set Valid: true
on these ones ✔️. Still, the utils function can be applied here for simplification as well:
wallet-backend/internal/utils/sql.go
Lines 8 to 13 in 8be1927
func SQLNullString(s string) sql.NullString { | |
return sql.NullString{ | |
String: s, | |
Valid: s != "", | |
} | |
} |
var ErrOperationNotFound = errors.New("operation not found") | ||
|
||
// TokenTransferProcessor processes Stellar transactions and extracts token transfer events. | ||
// It converts Stellar operations (payments, account merges, etc.) into standardized state changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 the word standardized seems to mean it's a kind of object that is standard across the Stellar ecosystem, which is not the case. I suggest updating the comment a bit:
// It converts Stellar operations (payments, account merges, etc.) into standardized state changes. | |
// It converts Stellar operations (payments, account merges, etc.) into our internal (synthetic) state changes representation. |
func (b *StateChangeBuilder) WithAsset(asset *asset.Asset, contractAddress string) *StateChangeBuilder { | ||
if asset != nil { | ||
if asset.GetNative() { | ||
b.base.Token = sql.NullString{String: "native"} | ||
} else if issuedAsset := asset.GetIssuedAsset(); issuedAsset != nil { | ||
b.base.Token = sql.NullString{String: fmt.Sprintf("%s:%s", issuedAsset.GetAssetCode(), issuedAsset.GetIssuer())} | ||
} | ||
} else { | ||
b.base.ContractID = sql.NullString{String: contractAddress} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would advocate for always using the contract id for tokens when possible, even for native or classic assets. For that same reason, maybe we change the name of the function to withToken
?
type StateChangeBuilder struct { | ||
base types.StateChange | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the withAsset
function, this builder class seems unnecessary and we can just use StateChange
, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming there will be more fields to set when we support more state change types, in which case this would help make things more readable in the processors.
// Process both fee events (transaction costs) and operation events (transfers, mints, etc.) | ||
allEvents := append(txEvents.FeeEvents, txEvents.OperationEvents...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From an ordering perspective, are we sure that we should have fee events proceed operation events? We may want to confirm with @karthikiyer56 or even a core engineer working on P23 how to think about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is precisely why we break out free events into their own segment when reading events from the events from transaction level.
The coalescing happens at the event from ledger level, where I break down the use cases based on pre-protocol 23 and post protocol 23 behavior.
As to whether you need to order events in the chronological order, depends entirely on what you might do with them.
If you simply store them in a DB, for retrieval purposes in the future , then you are probably okay.
You might still want to take a look at the latest code in the protocol 23 branch of TTP, and if needed, emulate that behavior in your own logic here
opID, opType, opSourceAccount, err = p.parseOperationDetails(tx, ledgerNumber, txIdx, opIdx) | ||
if err != nil { | ||
if errors.Is(err, ErrOperationNotFound) { | ||
// Skip events for operations that couldn't be found | ||
continue | ||
} | ||
return nil, fmt.Errorf("parsing operation details for transaction hash: %s, operation index: %d, err: %w", txHash, opIdx, err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would an operation not be found? Can't we assume that e.GetMeta().GetOperationIndex()
will always return a valid index for the transaction from which e
was produced? If so, then if this error is returned, it indicates a bug in our code, in which case we shouldn't fail silently and continue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
} | ||
|
||
// handleDefaultTransfer handles normal transfers and liquidity pool interactions. | ||
// For LP interactions, creates single state change with LP ID. For regular transfers, creates debit/credit pair. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should clarify why we're handling transfers from/to LPs here, because its not obvious. Explicit deposit and withdraw operations are handled in handleTransfer()
, but path payments could involve using an LP to swap assets.
// handleFee processes transaction fee events. | ||
// Positive amounts are debits (fee charged), negative amounts are credits (fee refunds). | ||
func (p *TokenTransferProcessor) handleFee(fee *ttp.Fee, contractAddress string, builder *StateChangeBuilder) ([]types.StateChange, error) { | ||
amount := fee.GetAmount() | ||
var category types.StateChangeCategory | ||
var finalAmount string | ||
|
||
// Negative fee amounts represent refunds (common in Soroban transactions) | ||
if strings.HasPrefix(amount, "-") { | ||
category = types.StateChangeCategoryCredit | ||
// Store the refund as positive credit amount | ||
finalAmount = strings.TrimPrefix(amount, "-") | ||
} else { | ||
// Positive fee amounts are normal transaction costs | ||
category = types.StateChangeCategoryDebit | ||
finalAmount = amount | ||
} | ||
|
||
change := builder.WithCategory(category). | ||
WithAccount(fee.GetFrom()). | ||
WithAmount(finalAmount). | ||
WithAsset(fee.GetAsset(), contractAddress). | ||
Build() | ||
|
||
return []types.StateChange{change}, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we decided whether we should consolidate fee events into a net fee? I know core breaks this down more granularly but I'm not familiar with the reasons why, and I think wallets are only interested in the net fee per transaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what can cause a negative fee (refund), it's the first time I see it 🤔
It's probably worth to follow @karthikiyer56's suggestion and consolidate it into a FeeChanges parameter that can be used to compute the resulting fee at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good!
My main suggestion is around reducing this scope a bit so we can get this PR merged sooner and then move forward to cases that require effects processing, like offers, path payments, and liquidity pools (already started in this PR).
if fromIsLP || toIsLP { | ||
if fromIsLP { | ||
// LP is sending tokens to account (e.g., path payment buying from LP) | ||
change := p.createLiquidityPoolChange(types.StateChangeCategoryCredit, to, from, amount, asset, contractAddress, builder) | ||
return []types.StateChange{change}, nil | ||
} | ||
// LP is receiving tokens from account (e.g., path payment selling to LP) | ||
change := p.createLiquidityPoolChange(types.StateChangeCategoryDebit, from, to, amount, asset, contractAddress, builder) | ||
return []types.StateChange{change}, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering it's not possible for both fromIsLP
and toIsLP
to be true, can we simplify things a bit?
if fromIsLP || toIsLP { | |
if fromIsLP { | |
// LP is sending tokens to account (e.g., path payment buying from LP) | |
change := p.createLiquidityPoolChange(types.StateChangeCategoryCredit, to, from, amount, asset, contractAddress, builder) | |
return []types.StateChange{change}, nil | |
} | |
// LP is receiving tokens from account (e.g., path payment selling to LP) | |
change := p.createLiquidityPoolChange(types.StateChangeCategoryDebit, from, to, amount, asset, contractAddress, builder) | |
return []types.StateChange{change}, nil | |
} | |
if fromIsLP { | |
// LP is sending tokens to account (e.g., path payment buying from LP) | |
change := p.createLiquidityPoolChange(types.StateChangeCategoryCredit, to, from, amount, asset, contractAddress, builder) | |
return []types.StateChange{change}, nil | |
} else if toIsLP { | |
// LP is receiving tokens from account (e.g., path payment selling to LP) | |
change := p.createLiquidityPoolChange(types.StateChangeCategoryDebit, from, to, amount, asset, contractAddress, builder) | |
return []types.StateChange{change}, nil | |
} |
// handleFee processes transaction fee events. | ||
// Positive amounts are debits (fee charged), negative amounts are credits (fee refunds). | ||
func (p *TokenTransferProcessor) handleFee(fee *ttp.Fee, contractAddress string, builder *StateChangeBuilder) ([]types.StateChange, error) { | ||
amount := fee.GetAmount() | ||
var category types.StateChangeCategory | ||
var finalAmount string | ||
|
||
// Negative fee amounts represent refunds (common in Soroban transactions) | ||
if strings.HasPrefix(amount, "-") { | ||
category = types.StateChangeCategoryCredit | ||
// Store the refund as positive credit amount | ||
finalAmount = strings.TrimPrefix(amount, "-") | ||
} else { | ||
// Positive fee amounts are normal transaction costs | ||
category = types.StateChangeCategoryDebit | ||
finalAmount = amount | ||
} | ||
|
||
change := builder.WithCategory(category). | ||
WithAccount(fee.GetFrom()). | ||
WithAmount(finalAmount). | ||
WithAsset(fee.GetAsset(), contractAddress). | ||
Build() | ||
|
||
return []types.StateChange{change}, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what can cause a negative fee (refund), it's the first time I see it 🤔
It's probably worth to follow @karthikiyer56's suggestion and consolidate it into a FeeChanges parameter that can be used to compute the resulting fee at the end.
case xdr.OperationTypeSetTrustLineFlags, xdr.OperationTypeAllowTrust: | ||
// Skip events generated by these operations since they involve only an LP and Claimable Balance ID | ||
return nil, nil | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Operations that pertain to liquidity pools, path payments and offers can all generate additional effects: modify funds on offers and liquidity pools. That's not entirely covered in this PR though and I don't think it should.
I think we could disable those operation types for now (as you did for OperationTypeAllowTrust and OperationTypeSetTrustLineFlags), and open a dedicated PR to handle them after we wrap this one. That way you'll simplify this PR and move this complexity to the next one, where you can focus solely on these specific cases.
What
Add a token transfer processor that uses the corresponding ttp processor to extract debit/credit events. Once the events are extracted, the processor generates state changes with the appropriate information (account ID, operation ID, tx hash, claimable balance ID etc....).
Note: only
DEBIT
,CREDIT
,MINT
andBURN
state changes are generated.The following operations are processed:
CreateAccount
AccountMerge
Payment
CreateClaimableBalance
ClaimClaimableBalance
Clawback
,ClawbackClaimableBalance
LiquidityPoolDeposit
,LiquidityPoolWithdraw
PathPaymentStrictSend
,PathPaymentStrictReceive
ManageBuyOffer
,ManageSellOffer
InvokeHostFunction (SEP-41)
The unit tests build a transaction for each case and asserts the proper creation of transfer events.
Why
Part of the state change indexer work
Known limitations
N/A
Issue that this PR addresses
Closes #194
Checklist
PR Structure
all
if the changes are broad or impact many packages.Thoroughness
Release