-
Notifications
You must be signed in to change notification settings - Fork 45
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
feat:update the lotus version #1153
Conversation
8e558f2
to
e56a544
Compare
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.
Left some more questions. https://github.com/filecoin-project/lily/pull/1153/files#diff-35818035b39b465ad281977acde860ea419bf987ddca1203c34f6904e3227dd7R25 will need to be addressed before this is ready for merge.
lens/lily/modules/statemanager.go
Outdated
@@ -22,7 +22,7 @@ var log = logging.Logger("lily/lens") | |||
var ErrExecutionTraceNotFound = fmt.Errorf("failed to find execution trace") | |||
|
|||
func StateManager(lmctx helpers.MetricsCtx, lc fx.Lifecycle, cs *store.ChainStore, exec stmgr.Executor, sys vm.SyscallBuilder, us stmgr.UpgradeSchedule, bs beacon.Schedule, em stmgr.ExecMonitor) (*stmgr.StateManager, error) { | |||
sm, err := stmgr.NewStateManagerWithUpgradeScheduleAndMonitor(cs, exec, sys, us, bs, em) | |||
sm, err := stmgr.NewStateManagerWithUpgradeScheduleAndMonitor(cs, exec, sys, us, bs, em, nil) |
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.
what is the nil
value being passed to the stmgr
? I assume it's a datastore? The statemanager will likely need a concrete value passed here to operate.
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.
Got it. But I am not familiar with it here. Could you help me to do it here? thanks!
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.
Here is the constructor from lotus: https://github.com/filecoin-project/lotus/blob/master/node/modules/stmgr.go#L13
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.
So this should look something like:
func StateManager(lmctx helpers.MetricsCtx, lc fx.Lifecycle, cs *store.ChainStore, exec stmgr.Executor, sys vm.SyscallBuilder, us stmgr.UpgradeSchedule, bs beacon.Schedule, em stmgr.ExecMonitor, metadataDs dtypes.MetadataDS) (*stmgr.StateManager, error) {
sm, err := stmgr.NewStateManagerWithUpgradeScheduleAndMonitor(cs, exec, sys, us, bs, em, metadataDs)
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.
This is the package lotus and lily use for dep injection like this: https://github.com/uber-go/fx
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.
Fixed! Thanks for your help!
.golangci.yml
Outdated
@@ -40,6 +40,9 @@ issues: | |||
- path: schemas/.* | |||
linters: | |||
- misspell | |||
- linters: | |||
- staticcheck | |||
text: "SA1019:" |
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.
what does this change do?
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.
we skip the lint check for old package: commands/daemon.go:6:2: SA1019: package io/ioutil is deprecated: As of Go 1.16, the same functionality is now provided by package io or package os, and those implementations should be preferred in new code. See the specific function documentation for details. (staticcheck)
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.
Instead of silencing the linter errors, replace the deprecated dependency: io/ioutil
with either the io
or os
package.
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.
Fixed!
…ject/lily into terryhung/upgrade-lotus-dep
Changes