-
Notifications
You must be signed in to change notification settings - Fork 102
fix #1248 changing the deal label to bytes not strings #1496
Conversation
Codecov Report
@@ 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 |
@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 |
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. |
There was a problem hiding this 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.
There was a problem hiding this 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
* 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
* 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>
fix #1248