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

fix: market: use expiry-activation for deal weight calculation #291

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jul 16, 2024

Ref: #237

This is the same as #237 but on top of master and with v14 included and v12 excluded.

It's a 👍 from me and @magik6k endorsed this on slack and it possibly (but maybe not) addresses a live issue being experienced by lotus-miner @ filecoin-project/lotus#12014

This apparently should have been done with the introduction of allocation term-min term-max.

As per my comment in #239 on the scope of this and what it touches:

In actors we calculate duration the same way: https://github.com/filecoin-project/builtin-actors/blob/8d957d2901c0f2044417c268f0511324f591cb92/actors/miner/src/lib.rs#L5522-L5534

This code gets used in lotus for StateMinerPreCommitDepositForPower here: https://github.com/filecoin-project/lotus/blob/21abfc69fb552b92fa3ce223e863f1d582dec237/node/impl/full/state.go#L1463-L1468

@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2024

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 3.57%. Comparing base (345ace6) to head (28a49a1).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #291   +/-   ##
======================================
  Coverage    3.57%   3.57%           
======================================
  Files         520     520           
  Lines       96959   96959           
======================================
  Hits         3468    3468           
  Misses      92140   92140           
  Partials     1351    1351           
Files Coverage Δ
builtin/v13/market/market_state.go 0.00% <0.00%> (ø)
builtin/v14/market/market_state.go 0.00% <0.00%> (ø)
builtin/v13/market/policy.go 0.00% <0.00%> (ø)
builtin/v14/market/policy.go 0.00% <0.00%> (ø)

@rjan90
Copy link
Contributor

rjan90 commented Jul 22, 2024

@aarshkshah1992 Could you give this PR a review today? As I would like to have this together with the final GST v0.14.0 release.

Copy link
Contributor

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

Changes here look good assuming we're okay changing the deal duration calculation like this. Looks like we already have pre-approval from Magik etc based on #237 (comment) so fine.

@rvagg rvagg merged commit 4e45acf into master Jul 22, 2024
9 checks passed
@rvagg rvagg deleted the rvagg/dealweight branch July 22, 2024 10:06
@rvagg rvagg mentioned this pull request Jul 23, 2024
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.

5 participants