Skip to content
This repository has been archived by the owner on Jun 6, 2023. It is now read-only.

Baseline migration #1215

Merged
merged 2 commits into from
Oct 6, 2020
Merged

Baseline migration #1215

merged 2 commits into from
Oct 6, 2020

Conversation

ZenGround0
Copy link
Contributor

The PR is motivated by the cryptoecon effort to fix some outstanding reward logic issues introduced in the 94000 upgrade.

@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2020

Codecov Report

Merging #1215 into master will increase coverage by 0.0%.
The diff coverage is 90.3%.

@@          Coverage Diff           @@
##           master   #1215   +/-   ##
======================================
  Coverage    71.0%   71.1%           
======================================
  Files          65      69    +4     
  Lines        6772    6800   +28     
======================================
+ Hits         4814    4837   +23     
- Misses       1183    1186    +3     
- Partials      775     777    +2     

Copy link
Member

@davidad davidad left a comment

Choose a reason for hiding this comment

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

This looks perfect so far!

@ZenGround0 ZenGround0 marked this pull request as ready for review October 5, 2020 20:20
@ZenGround0 ZenGround0 requested a review from anorth October 5, 2020 20:21
@ZenGround0
Copy link
Contributor Author

@anorth these things are decoupled from the raw math and worth your review (especially the first bullet)

  • Moving SimpleTotal and BaselineTotal on chain
  • Deferring reward actor migration to order after power actor
  • Refactoring math: moving functions into math package, renaming Precision to Precision128

It will help to read this commit by commit

@ZenGround0 ZenGround0 changed the title WIP Rough draft baseline migration Bbaseline migration Oct 5, 2020
@ZenGround0 ZenGround0 changed the title Bbaseline migration Baseline migration Oct 5, 2020
Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

Looks good as far as I can judge, after one fix.

actors/util/math/exp.go Outdated Show resolved Hide resolved
@arajasek arajasek mentioned this pull request Oct 6, 2020
21 tasks
Copy link
Member

@davidad davidad left a comment

Choose a reason for hiding this comment

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

I have reviewed all the math. Everything looks great, except for one missing digit and a missed data dependency I had failed to recognize in the spec.

actors/builtin/reward/reward_logic.go Show resolved Hide resolved
actors/migration/reward.go Outdated Show resolved Hide resolved
actors/migration/reward.go Outdated Show resolved Hide resolved
actors/migration/reward.go Outdated Show resolved Hide resolved
actors/migration/reward.go Show resolved Hide resolved
actors/migration/reward.go Show resolved Hide resolved
actors/migration/reward.go Show resolved Hide resolved
actors/migration/reward.go Show resolved Hide resolved
actors/migration/reward.go Show resolved Hide resolved
actors/util/math/ln.go Show resolved Hide resolved
- Migrate reward actor state
- Cleanup: math.Precision -> math.Precision128
- Cleanup: move ln from smoothing to math
- Cleanup: move expneg from reward to math
- Hook up reward actor with migrated power state
- Move SimpleTotal, BaselineTotal on chain
- Compute new baseline total in migration
Copy link
Member

@davidad davidad left a comment

Choose a reason for hiding this comment

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

All issues have been fixed, looks great to me now. Thank you @ZenGround0!

@ZenGround0 ZenGround0 merged commit 385dfad into master Oct 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants