Skip to content
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

Include claimable balances in Asset Stats endpoint #3476

Closed
bartekn opened this issue Mar 19, 2021 · 3 comments · Fixed by #3502
Closed

Include claimable balances in Asset Stats endpoint #3476

bartekn opened this issue Mar 19, 2021 · 3 comments · Fixed by #3502
Assignees
Labels
Milestone

Comments

@bartekn
Copy link
Contributor

bartekn commented Mar 19, 2021

Asset stats (/assets) don't include claimable balances. We probably should add number of claimable balances holding a given asset with the sum amount. This can be either part of #3454 or in a separate PR.

@bartekn bartekn added the bug label Mar 19, 2021
@bartekn bartekn added this to the Horizon 2.1.0 milestone Mar 19, 2021
@paulbellamy paulbellamy self-assigned this Mar 25, 2021
@paulbellamy
Copy link
Contributor

paulbellamy commented Mar 25, 2021

Initial thought was to add this into the AssetStatsProcessor. The process being roughly:

  • When a claimable balance is created we subtract the amount from the creator trustline.
  • When it is claimed we add the balance to the destination trustline
  • When it is clawed back we add the balance back to the creator trustline

However, that might have the same issue as #3380 and #3306, where the ingest.Change doesn't include any information about the claimable balance's creator. 🤔 The difference being that in the processor we have access to the DB, so we could query that info from the history_claimable_balance_transactions table. But I don't like the idea of having the ingesters be coupled via the db.

Second thought, is we could do it dynamically in the /assets action. But, that means to load the /assets endpoint we also have to query all the current claimable balances, and do some math on the fly to calculate the new balances and accounts. We do the same for issuers already, but those are guaranteed to be 1:1 with assets. I worry about that having performance implications if there are a lot of claimable balances on-the-go.

@tamirms
Copy link
Contributor

tamirms commented Mar 25, 2021

@paulbellamy I believe you should be able to implement it in the AssetStatsProcessor. You don't need to have any logic to check who created the claimable balance or how the claimable balance was claimed or clawed back.

When you create a claimable balance I think there will be two ingest.Change produced from that operation. The first change will correspond to the trustline balance change of the creator. The other change will correspond to the event of a new claimable balance being introduced in the ledger.

Similarly when you claim or clawback a claimable balance, that operation should produce two ingest.Change instances. The first change will correspond to a trustline balance increasing. The other change will correspond to the claimable balance being removed from the ledger.

I think these are the only cases you need to handle:

		// only handle changes involving claimable balances
		switch {
		case change.Pre == nil && change.Post != nil:
			// Created
			newCb := change.Post.Data.MustClaimableBalance()
			// increase claimable balance stats
		case change.Pre != nil && change.Post != nil:
			// claimable balances are immutable so this case should never happen
		case change.Pre != nil && change.Post == nil:
			// Removed
			oldCb := change.Pre.Data.MustClaimableBalance()
			// decrease claimable balance stats
		default:
			return errors.New("Invalid io.Change: change.Pre == nil && change.Post == nil")
		}

@bartekn
Copy link
Contributor Author

bartekn commented Mar 25, 2021

^ What @tamirms said, just one quick note on:

// claimable balances are immutable so this case should never happen

I think there's one case in which it's possible: when the sponsor is changed. But this won't affect the amount nor the asset.

@bartekn bartekn modified the milestones: Horizon 2.1.0, Horizon 2.2.0 Mar 31, 2021
paulbellamy pushed a commit that referenced this issue Apr 1, 2021
…ats summaries (#3502)

* Refactor AssetStatProcessor to make space for handling new change types

* WIP --adding claimable balances

* Fix two bugs

* Add support for claimable balance in processor

also add tests

* Add API implementation and tests

* attempting to refactor

* Handle special case of native asset

* Update docs

* update CHANGELOG

* Fixing test

* rename function

* Refactor AssetStatSet

* improve commit functions

* Fix godoc strings

* Update services/horizon/internal/docs/reference/endpoints/assets-all.md

Co-authored-by: Bartek Nowotarski <[email protected]>

* Update changelog

* Move claimable balance balance and accounts out of the structs

* use ingest.Change in assetStatSet.Add* methods

Co-authored-by: tamirms <[email protected]>
Co-authored-by: Bartek Nowotarski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants