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

Not to merge: rename circulating supply to total supply #1461

Closed
wants to merge 1 commit into from

Conversation

alexytsu
Copy link
Contributor

@alexytsu alexytsu commented Oct 25, 2023

For context:

#1460
filecoin-project/fvm-workbench#33

Total supply should be calculable by summing up all actors' balances.

pub fn check_invariants(vm: &dyn VM, policy: &Policy) -> anyhow::Result<MessageAccumulator> {
    check_state_invariants(
        &vm.actor_manifest(),
        policy,
        Tree::load(&DynBlockstore::wrap(vm.blockstore()), &vm.state_root()).unwrap(),
        &vm.total_supply(),
        vm.epoch() - 1,
    )
}

It's current use in check_invariants is probably conceptually wrong. An externally calculated/predetermined total_supply should be passed as a parameter into check_invariants. This raises the question whether set_total_supply needs to exist.

The check_invariants probably doesn't need to do the total_supply check. It should be enforced by the VM. Since the TestVM is implemented here however, there's value in testing its compliance - but that should be in specific unit tests of the TestVM rather than in every integration test (that should target the actors behaviour).

I think the VM should expose:

  • set_circulating_supply
  • circulating_supply (simply read from set_circulating_supply)
  • optionally: a dynamically calculated total_supply method. I don't think this is used in any test bodies at the moment.

@alexytsu alexytsu requested a review from anorth October 25, 2023 06:03
@codecov-commenter
Copy link

Codecov Report

Merging #1461 (7388b6e) into master (1e50065) will decrease coverage by 0.06%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1461      +/-   ##
==========================================
- Coverage   91.05%   91.00%   -0.06%     
==========================================
  Files         147      147              
  Lines       27982    27982              
==========================================
- Hits        25479    25465      -14     
- Misses       2503     2517      +14     
Files Coverage Δ
integration_tests/src/util/mod.rs 100.00% <100.00%> (ø)
test_vm/src/lib.rs 96.07% <100.00%> (ø)
vm_api/src/lib.rs 91.66% <ø> (ø)

... and 2 files with indirect coverage changes

@anorth
Copy link
Member

anorth commented Oct 25, 2023

I think the VM should expose:
set_circulating_supply
circulating_supply (simply read from set_circulating_supply)
optionally: a dynamically calculated total_supply method. I don't think this is used in any test bodies at the moment.

I agree with all of these, and thus not with the code change in this draft PR. Adding a parameter to check_invariants giving the expected total supply (optional) sounds good. Most actor tests might not know it and will pass None, but some other uses (checking actual mainnet state) will know the expected value.

@alexytsu
Copy link
Contributor Author

Replaced by #1466

@alexytsu alexytsu closed this Oct 26, 2023
@alexytsu alexytsu deleted the alexytsu/total-supply branch October 26, 2023 03:04
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.

3 participants