-
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
api: Separate the Net interface from Common #6627
Conversation
b0af7d2
to
94bfe71
Compare
@@ -41,6 +41,7 @@ import ( | |||
// StorageMiner is a low-level interface to the Filecoin network storage miner node | |||
type StorageMiner interface { | |||
Common | |||
Net |
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 don't think we have to do this in initial MRA split PR, but ideally:
- We'd have the miner interface split into more, per service interfaces
- In the API serve logic we'd only construct the RPC server with the services a node actually supports
"github.com/filecoin-project/lotus/node/modules/dtypes" | ||
"github.com/filecoin-project/lotus/storage" | ||
"github.com/filecoin-project/lotus/storage/sectorblocks" | ||
sto "github.com/filecoin-project/specs-storage/storage" | ||
) | ||
|
||
type StorageMinerAPI struct { | ||
common.CommonAPI | ||
fx.In |
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.
@magik6k why is this necessary now?
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.
It's no longer provided by the embedded structs (which Fx apparently was picking up)
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.
(fx.In
embeds dig.In
, which is a private interface, so if it was embedded in deeper structs, the parent struct would 'inherit' it)
e2cf25a
to
87b1029
Compare
87b1029
to
812dc26
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.
LGTM
return a.Host.Network().Connectedness(pid), nil | ||
} | ||
|
||
func (a *Libp2pNetAPI) NetPubsubScores(context.Context) ([]api.PubsubScore, error) { | ||
func (a *NetAPI) NetPubsubScores(context.Context) ([]api.PubsubScore, 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.
super nit: it would be nice if the order of methods here matched the order of declarations on the Net interface in api_net.go
b85c09b
to
d5c9ccc
Compare
This implements the basic separation of the Net api from CommonAPI, which should make it much easier to construct nodes without it
(this is for #6356, but can be merged separately to reduce PR sizes)