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

chore: update to lotus 1.5.0-pre1 and actors v3 #371

Merged
merged 1 commit into from
Jan 27, 2021
Merged

Conversation

iand
Copy link
Contributor

@iand iand commented Jan 26, 2021

Tests for actors v3 are copy/paste of v2 tests with fixups for the types. The main change for v3 is the structure of the HAMT.

@iand iand force-pushed the chore/lotus-150-pre branch 3 times, most recently from 673ea54 to d79fb3a Compare January 26, 2021 13:43
@iand iand changed the title chore: update imports and ffi stub for lotus 1.5.0-pre1 chore: update to lotus 1.5.0-pre1 and actors v3 Jan 26, 2021
@codecov-io
Copy link

codecov-io commented Jan 26, 2021

Codecov Report

Merging #371 (e5fd69e) into master (0f5a496) will increase coverage by 0.0%.
The diff coverage is 41.1%.

@@          Coverage Diff           @@
##           master    #371   +/-   ##
======================================
  Coverage    43.0%   43.1%           
======================================
  Files          25      25           
  Lines        1917    1932   +15     
======================================
+ Hits          826     833    +7     
- Misses        964     972    +8     
  Partials      127     127           

@iand iand requested review from frrist and placer14 and removed request for placer14 and frrist January 26, 2021 14:39
@iand iand requested review from frrist and placer14 January 26, 2021 14:41
@placer14 placer14 self-assigned this Jan 26, 2021
Copy link
Contributor

@placer14 placer14 left a comment

Choose a reason for hiding this comment

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

Confused how the ffi-stub was building before the types or constants were present. I don't see where it was changed but checking out locally it obviously points to that module. Just some small confusion, but looks good otherwise. 🤝

@@ -41,7 +43,7 @@ func TestRewardExtractV0(t *testing.T) {
stateCid, err := mapi.Store().Put(ctx, state)
require.NoError(t, err)

minerAddr := tutils.NewIDAddr(t, 00)
minerAddr := tutils.NewIDAddr(t, 0o0)
Copy link
Contributor

Choose a reason for hiding this comment

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

May be nothing, but curious why this was needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not my doing. I think gofumpt does it. Numbers starting with 0 are octal in Go, and they added 0o as a consistent way to write them to match 0x

@willscott
Copy link
Contributor

only the subset of ffi stub actually referenced by us or lotus is needed when a module replacement happens.

Copy link
Member

@frrist frrist left a comment

Choose a reason for hiding this comment

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

LGTM! 🙏

@willscott
Copy link
Contributor

ℹ️ There is a dependency on statediff for parsing message parameters. that may also need to get pulled up to the v3 dependencies to continue functioning. I expect to do that this week.

@iand
Copy link
Contributor Author

iand commented Jan 26, 2021

Confused how the ffi-stub was building before the types or constants were present. I don't see where it was changed but checking out locally it obviously points to that module. Just some small confusion, but looks good otherwise.

They were in the real ffi package but not imported into Lotus until recently.

@iand iand merged commit 05f50b5 into master Jan 27, 2021
@iand iand deleted the chore/lotus-150-pre branch January 27, 2021 12:06
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