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

fix #1248 changing the deal label to bytes not strings #1496

Merged
merged 11 commits into from
Oct 14, 2021
Merged

Conversation

laudiacay
Copy link
Contributor

fix #1248

@laudiacay laudiacay requested a review from a team as a code owner September 24, 2021 17:33
@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2021

Codecov Report

Merging #1496 (26d6f2a) into next (7a69070) will increase coverage by 0.0%.
The diff coverage is 90.3%.

@@          Coverage Diff          @@
##            next   #1496   +/-   ##
=====================================
  Coverage   71.4%   71.5%           
=====================================
  Files         72      73    +1     
  Lines       8546    8559   +13     
=====================================
+ Hits        6109    6120   +11     
- Misses      1553    1554    +1     
- Partials     884     885    +1     

@ZenGround0 ZenGround0 changed the base branch from master to next September 24, 2021 17:49
@ZenGround0
Copy link
Contributor

@laudiacay this is a good code change. There is an associated data change to mainnet state that will need to also be made through an "actors state migration". See this example PR for one example of how this looks: https://github.com/filecoin-project/specs-actors/pull/1265/files

@ZenGround0
Copy link
Contributor

This is also something that will need a Fillecoin Improvement Proposal (FIP) to signal the change and give time to implementers of external tooling around deal data. This process takes place here: https://github.com/filecoin-project/FIPs.

@laudiacay laudiacay marked this pull request as ready for review October 5, 2021 20:37
@laudiacay laudiacay requested a review from ZenGround0 October 5, 2021 20:37
Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

There are a few tricky bugs that need to be fixed for this to be correct but it is almost there.

The most work remaining is to add a test of the migration over market state that actually contains proposals. This test should include unactivated, activated but not yet cronned and fully activated and cronned deals. The place for this test is in a new file actors/migration/nv14/test/deal_label_migration_test.go. It will combine the deal making and sector commitment of this test with the migration over test state found in [the parallel migration call] you've already debugged.

actors/builtin/market/deal.go Outdated Show resolved Hide resolved
actors/migration/nv14/top.go Show resolved Hide resolved
actors/migration/nv14/market.go Outdated Show resolved Hide resolved
actors/migration/nv14/market.go Outdated Show resolved Hide resolved
actors/migration/nv14/market.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

LGTM especially if you add a check that the proposal label field is unchanged apart from types.

Tests are also failing

actors/migration/nv14/test/deal_label_migration_test.go Outdated Show resolved Hide resolved
actors/migration/nv14/test/deal_label_migration_test.go Outdated Show resolved Hide resolved
actors/migration/nv14/test/deal_label_migration_test.go Outdated Show resolved Hide resolved
actors/migration/nv14/test/deal_label_migration_test.go Outdated Show resolved Hide resolved
actors/migration/nv14/test/deal_label_migration_test.go Outdated Show resolved Hide resolved
@laudiacay laudiacay merged commit b91ffa9 into next Oct 14, 2021
@laudiacay laudiacay deleted the fix-1248 branch October 14, 2021 16:55
ZenGround0 pushed a commit that referenced this pull request Dec 31, 2021
* changing the deal label to bytes not strings

* removing a todo comment for something that was actually a to-done

* wrote migration.... how do i test??

* fixing tests, determinism-gen

* left some debugging prints in there

* fixes some of the code review

* fixing one more issue

* fixing rest of code review things- all that's left isss testing

* half a test

* there's a bug in the migration, test is done

* fixing code review
arajasek pushed a commit that referenced this pull request Jan 3, 2022
* changing the deal label to bytes not strings (#1496)


Co-authored-by: c r <c@laudiacay.cool>
Co-authored-by: zenground0 <ZenGround0@users.noreply.github.com>
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.

DealProposal's Label field serializes as non-UTF8
4 participants