update for the routing refactor #148
Conversation
ghost
assigned 
dht.go
Outdated
| @@ -67,8 +66,8 @@ type IpfsDHT struct { | |||
| } | |||
|
|
|||
| // NewDHT creates a new DHT object with the given peer as the 'local' host | |||
| func NewDHT(ctx context.Context, h host.Host, dstore ds.Batching) *IpfsDHT { | |||
| dht := NewDHTClient(ctx, h, dstore) | |||
| func NewDHT(ctx context.Context, h host.Host, dstore ds.Batching, validator record.Validator) *IpfsDHT { | |||
There was a problem hiding this comment.
I am not sure I like having a required record validator argument; it will also flat out break existing code.
Can we make this optional with a variadic?
There was a problem hiding this comment.
The default should be what the old default functionality was, public key validation so that it's backwards compatible.
There was a problem hiding this comment.
Yeah, good point. I was in a "get shit working at all costs" mood when I made this change.
What about just going back to exposing a Validator public field. That's closer to what we currently do (although racy if the host is up and accepting streams). The nice alternative is an options system (more code but probably better in the long run).
There was a problem hiding this comment.
let's make it nice, there is no rush.
dht.go
Outdated
| @@ -77,7 +76,7 @@ func NewDHT(ctx context.Context, h host.Host, dstore ds.Batching) *IpfsDHT { | |||
| } | |||
|
|
|||
| // NewDHTClient creates a new DHT object with the given peer as the 'local' host | |||
| func NewDHTClient(ctx context.Context, h host.Host, dstore ds.Batching) *IpfsDHT { | |||
| func NewDHTClient(ctx context.Context, h host.Host, dstore ds.Batching, validator record.Validator) *IpfsDHT { | |||
|
@vyzo I've added a new DHT constructor that takes options. This also unifies the DHT and DHTClient constructors and configures a default datastore (allows users to start using libp2p without understanding datastores). |
dht.go
Outdated
| @@ -65,19 +66,13 @@ type IpfsDHT struct { | |||
| plk sync.Mutex | |||
| } | |||
|
|
|||
| // NewDHT creates a new DHT object with the given peer as the 'local' host | |||
| func NewDHT(ctx context.Context, h host.Host, dstore ds.Batching, validator record.Validator) *IpfsDHT { | |||
There was a problem hiding this comment.
keep those around for backwards compatibility. it is not acceptable to have to change existing working code.
There was a problem hiding this comment.
i mean the original constructor that takes a context, host, and datastore.
There was a problem hiding this comment.
I did. I'm just reverting my change to them here. There are now three constructors:
NewDHT(old)NewDHTClient(old)New(new)
dht.go
Outdated
| } | ||
|
|
||
| // NewDHTClient creates a new DHT object with the given peer as the 'local' host | ||
| func NewDHTClient(ctx context.Context, h host.Host, dstore ds.Batching, validator record.Validator) *IpfsDHT { |
Stebalien
force-pushed
the
feat/routing-refactor
branch
from
5565beb
to
ec3bc1d
Compare
GetValues was very DHT specific so the routing interface has been updated to remove that function. Instead, it has introduced general-purpose options. This is a minimal alternative to #141 to avoid bundling too many changes together.
Note: this does mean that the DHT won't work with peer keys by default and that the constructor signature changes. Given all the changes that'll come with the libp2p refactor, I don't feel too bad about this.
Instead of changing the existing constructors, add a new DHT constructor that takes options (and add DHT options).
Stebalien
force-pushed
the
feat/routing-refactor
branch
from
ec3bc1d
to
c0d3351
Compare
GetValues was very DHT specific so the routing interface has been updated to remove that function. Instead, it has introduced general-purpose options.
This is a "minimal" alternative to #141 to avoid bundling too many changes together.
Depends on: