services/horizon: Include contract asset balances in asset stats #4805
Conversation
| // Action needed in release: horizon-v3.0.0: deprecated field | ||
| Amount string `json:"amount"` | ||
| Accounts AssetStatAccounts `json:"accounts"` | ||
| ClaimableBalancesAmount string `json:"claimable_balances_amount"` | ||
| LiquidityPoolsAmount string `json:"liquidity_pools_amount"` | ||
| ContractsAmount string `json:"contracts_amount"` |
There was a problem hiding this comment.
Is this the total amount of assets in all the contracts? Then I would rename it to something like AllContractsAmount or ContractsTotalAmount or ContractsAmountSum
There was a problem hiding this comment.
I think this is the amount of the given, specific asset that is stored w/in its SAC contract. Or does it go beyond the SAC, @tamirms?
There was a problem hiding this comment.
@2opremio is correct that is the total amount held by all smart contracts. I agree that renaming it would be better but then it would be inconsistent with liquidity_pools_amount and claimable_balances_amount. We could rename all the fields but that would be a breaking change
| // Action needed in release: horizon-v3.0.0: deprecated field | ||
| Amount string `json:"amount"` | ||
| Accounts AssetStatAccounts `json:"accounts"` | ||
| ClaimableBalancesAmount string `json:"claimable_balances_amount"` | ||
| LiquidityPoolsAmount string `json:"liquidity_pools_amount"` | ||
| ContractsAmount string `json:"contracts_amount"` |
There was a problem hiding this comment.
I think this is the amount of the given, specific asset that is stored w/in its SAC contract. Or does it go beyond the SAC, @tamirms?
| amount := amountObj.MustI128() | ||
| // check if negative | ||
| if ((amount.Hi >> 56) & 0x80) > 0 { | ||
| return [32]byte{}, nil, false | ||
| } |
There was a problem hiding this comment.
😳 is there a test for this? why 56?
There was a problem hiding this comment.
should we also check that Hi or Lo fits in an int64?
There was a problem hiding this comment.
wondered similar, for balances,Hi is just holding sign extension bits for Lo as uint64? just the most significant bit in Hi could be checked?
services/horizon/internal/ingest/processors/asset_stats_processor.go
Outdated
Show resolved
Hide resolved
services/horizon/internal/ingest/processors/asset_stats_processor.go
Outdated
Show resolved
Hide resolved
| if obj.Type != xdr.ScObjectTypeScoBytes { | ||
| return nil | ||
| } | ||
| var err error | ||
| assetIssuer, err = strkey.Encode(strkey.VersionByteAccountID, obj.MustBin()) |
There was a problem hiding this comment.
I think this is wrong :
if objMjustBin() is nil ,then you shoudl return nil.
however, if strkey.Encode returns an error, you should return this error.
There was a problem hiding this comment.
if the issuer byte array is invalid then this is an indication that it's not coming from the stellar asset contract which is why I return nil. But you're right that I should check that obj.MustBin() is not nill
services/horizon/internal/ingest/processors/asset_stats_processor.go
Outdated
Show resolved
Hide resolved
|
@stellar/horizon-committers PTAL I think I've addressed all the code review feedback |
| return [32]byte{}, nil, false | ||
| } | ||
| amt := new(big.Int).Lsh(big.NewInt(int64(amount.Hi)), 64) | ||
| amt.Add(amt, big.NewInt(int64(amount.Lo))) |
There was a problem hiding this comment.
Based on my discussion on Slack here, I think int64(amount.Lo) is not safe. You should do big.NewInt(0).SetUint64(amount.Lo), instead, because only the upper half contains info about the overall sign of amount. Otherwise this will give you a negative value if amount > MaxInt64
There was a problem hiding this comment.
this is a great catch. I think there is a bug here. I will fix this and update the tests to catch this
There was a problem hiding this comment.
in general, we should probably shove this into a library wrapper around Int128Parts as these conversions/math can easily be a huge source of bugs down the line
There was a problem hiding this comment.
I made #4809 to track this for the future.
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
.mdfiles, etc... affected by this change). Take a look in the
docsfolder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
This PR a follow up to #4782 where we update asset stats ingestion to keep track of how many contracts hold an asset and the amount of an asset held by all contracts in total.
This is implemented by ingesting balance contract data ledger entries which are updated by the stellar asset contract. The balance contract data ledger entries are like trustlines but for contract holders of an asset.
Why
This PR is required to implement #4727
Known limitations
We cannot directly identify if a balance contract data ledger entry belongs to a genuine stellar asset contract. So we work around this by querying the asset stats table by contract id. If there is a row present then we know that the balance contract data ledger entry corresponds to the stellar asset contract. Otherwise, we ignore the ledger entry change in ingestion.
If the extra db load becomes a problem we could think about having a something like a bloom filter which we can use to discard ledger entries from non stellar asset contracts that mimic the stellar asset contract balance format.