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

Add a StateExactCirculatingSupply command #4148

Merged
merged 2 commits into from Oct 12, 2020
Merged

Add a StateExactCirculatingSupply command #4148

merged 2 commits into from Oct 12, 2020

Conversation

arajasek
Copy link
Contributor

@arajasek arajasek commented Oct 3, 2020

At height 136,374, circ supply is 15579755.735329679830248779 FIL

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic looks good

api/api_full.go Outdated
@@ -400,6 +400,9 @@ type FullNode interface {

// StateCirculatingSupply returns the circulating supply of Filecoin at the given tipset
StateCirculatingSupply(context.Context, types.TipSetKey) (CirculatingSupply, error)
// TODO: Remove StateCirculatingSupply maybe?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used by chainwatch/visor to put this data into postgres, which the CE team can query, so probably not

We could add a comment to StateCirculatingSupply saying that it's an approximation, and that people should use StateCirculatingSupply in most cases

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*StateExactCirculatingSupply

cli/state.go Outdated
@@ -1638,6 +1639,34 @@ var stateCircSupplyCmd = &cli.Command{
},
}

// TODO: Remove stateCircSupplyCmd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd

  • Make this the default output of circulating-supply
  • Add a --vm-supply flag to get the api.StateCirculatingSupply one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sounds like the approx CircSupply isn't going anywhere, so I will do this

func (sm *StateManager) GetExactCirculatingSupply(ctx context.Context, height abi.ChainEpoch, st *state.StateTree) (abi.TokenAmount, error) {
circ := big.Zero()
unCirc := big.Zero()
err := st.ForEach(func(a address.Address, actor *types.Actor) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should skip further processing on actors with zero balance

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

a == builtin.SystemActorAddr ||
a == builtin.CronActorAddr ||
a == builtin.BurntFundsActorAddr ||
a == builtin.SaftAddress ||
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could start including funds in this address post-liftoff, but eh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or just include it now, there's only 4M in it.

@arajasek arajasek force-pushed the asr/circ-supply branch 2 times, most recently from 971985a to 4762430 Compare October 11, 2020 06:30
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not being completely in the know about the rationale behind these changes (the PR description doesn't explain much about the motivation of this change, which makes it hard to follow), it looks like a smell that:

  1. we have two circulating supplies,
  2. we're choosing to expose the "exact" one via the API by default, but letting the VM use the non-exact value.

Intuitively, something feels a bit off here, either the divergence in values, or the naming. The circulating supply is a crucial cryptoecon variable, and IMO it should be as minimally ambiguous as possible.

@arajasek
Copy link
Contributor Author

@raulk You're not wrong, as I understand it, we're kinda stuck between two requirements here.

  • We must have an accurate calculation of the liquid FIL in the network. This is necessarily slow, but is the correct number to report to anyone external who wants to know how much FIL is circulating.
  • We must have a quick approximation of the liquid FIL in the network. This is needed to be used internally when doing CE math (and so by the VM).

The naming is intended to reflect that external parties probably wanna use the slow, accurate method. Very open to hearing recommendations, though.

@raulk
Copy link
Member

raulk commented Oct 11, 2020

@arajasek

This is needed to be used internally when doing CE math (and so by the VM).

Apologies for ignorance, what does CE mean here?

@arajasek
Copy link
Contributor Author

@arajasek

This is needed to be used internally when doing CE math (and so by the VM).

Apologies for ignorance, what does CE mean here?

CryptoEcon, sorry

@arajasek arajasek merged commit 195d96c into master Oct 12, 2020
@arajasek arajasek deleted the asr/circ-supply branch October 12, 2020 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants