-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Implement Event observer and Settings for 3rd party dep injection #5693
Conversation
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.
First pass - looks pretty good
(I'm assuming you want a review from @iand before merging)
Makefile
Outdated
lotus-sentinel: $(BUILD_DEPS) | ||
rm -f lotus-sentinel | ||
go build -o lotus-sentinel ./cmd/lotus-sentinel | ||
.PHONY: lotus-keygen |
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.
.PHONY: lotus-keygen | |
.PHONY: lotus-sentinel |
cmd/lotus-sentinel/daemon.go
Outdated
}, | ||
} | ||
|
||
func ImportChain(ctx context.Context, r repo.Repo, fname string, snapshot bool) (err error) { |
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.
I'd try to deduplicate this with the 'normal' daemon
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.
Any preference for where this should live? In order for other packages to use it, we'll have to move it out of this file since it's in package main
-- my first thought was the repo package.
This could also just be dropped as a feature from the lotus-sentinel binary and users would be required to perform the import with the lotus bin, thoughts @iand?
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.
Would prefer not to have to make the user switch between binaries for this task. Let's find a new home for this function in a suitable lotus package.
node/builder.go
Outdated
@@ -14,6 +14,7 @@ import ( | |||
"github.com/filecoin-project/lotus/chain/store" | |||
"github.com/filecoin-project/lotus/chain/vm" | |||
"github.com/filecoin-project/lotus/chain/wallet" | |||
api2 "github.com/filecoin-project/lotus/cmd/lotus-sentinel/api" |
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.
api2 "github.com/filecoin-project/lotus/cmd/lotus-sentinel/api" | |
sentinelapi "github.com/filecoin-project/lotus/cmd/lotus-sentinel/api" |
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.
Pretty happy with how this turned out. Now the question is whether the binary needs to live in the lotus repo or can it live in sentinel's? The extension points and exposure of things like ImportChain obviously have to stay in Lotus but once we have those it seems by moving the binary elsewhere we could avoid Lotus depending on the all the visor and postgres packages. Plus we get to decouple the development processes of the two projects.
cmd/lotus-sentinel/daemon.go
Outdated
}, | ||
} | ||
|
||
func ImportChain(ctx context.Context, r repo.Repo, fname string, snapshot bool) (err error) { |
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.
Would prefer not to have to make the user switch between binaries for this task. Let's find a new home for this function in a suitable lotus package.
I'm going to extract the lotus-sentinel binary code to the visor repo and modify this PR only to contain the changes required to make that work -- I suspect they will be minimal (sorry for the churn) |
lotus-sentinel
binary for sentinel integration
Converting to a draft as this will require more changes than I anticipated. The other half lives here: filecoin-project/lily#408 |
- allows tipset apply and revert to be observed
- motivating use case running a lotus-sentinel node
What