-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add extended providers support #292
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #292 +/- ##
==========================================
- Coverage 44.64% 44.55% -0.10%
==========================================
Files 78 80 +2
Lines 6018 6202 +184
==========================================
+ Hits 2687 2763 +76
- Misses 2907 3011 +104
- Partials 424 428 +4
|
35cc24e
to
d718b4c
Compare
@gmelodie @hannahhoward this PR adds support for provider families. Please have a look whether it's sufficient for your usecase. Or would you need to have CLI support too? |
go.mod
Outdated
github.com/filecoin-project/go-state-types v0.1.0 | ||
github.com/filecoin-project/storetheindex v0.4.28 | ||
github.com/filecoin-project/storetheindex v0.4.30-0.20221109112339-8dad1e315c7e |
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 will update the reference before merging the PR.
@@ -0,0 +1,155 @@ | |||
package extended_provider |
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 rationale for separating the package? Would be nice if we don't have to do this and the functionality simply extends the existing 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.
My original idea was to add this API into the main interface, however if you think about it - there is nothing fundamentally new that has been added. I.e. the core engine functionality and the APIs that are exposed from it already are sufficient for the extended providers usecase too.
Hence instead of modifying the existing APIs that would require at least 2 new parameters (override and ExtendedProviders) I decided to go with a layer on top of the existing engine. I also envision that the extended providers APIs are going to be used way less frequent comparing to the main Publish
and NotifyPut
. So modifying those to satisfy extended providers usecase was not an option either.
// | ||
// - contextID and metadata fields are optional and should be used according to the specification | ||
// - providers list should contain only those providers whos infos have been registered in the publisher | ||
func (pub *ExtendedProvidersPublisher) Publish(ctx context.Context, contextID []byte, metadata []byte, override bool, providers ...peer.ID) (cid.Cid, 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.
Wy not take Advertisement straight up? The number of arguments to this function seems to be a tad high.
If the idea is to assist with construction of Advertisement
then maybe it would be more intuitive to abstract away AdvertisementBuilder
instead of Publish
.
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 thought about the builder. The issue is that assistance is required not just in the building, but in the signing too. Would your concern be alleviated if instead of passing in providers as a multiarg, I use array instead?
It's just +1 argument to what NotifyPut
takes currently:
NotifyPut(ctx context.Context, provider *peer.AddrInfo, contextID []byte, md metadata.Metadata) (cid.Cid, 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 thought about the builder while jogging and liked the idea :) Pushed the change in.
@masih ready for re-review |
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 must insist on package and type name changes. See comments.
@gammazero fixes pushed please re-review |
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.
A couple more suggested changes, but not blocking. Do please change the directory name.
Tagging @elijaharita |
be80da9
to
2991f96
Compare
// Priv contains a provtae key of the extended provider | ||
Priv crypto.PrivKey |
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.
// Priv contains a provtae key of the extended provider | |
Priv crypto.PrivKey | |
// Priv contains a private key of the extended provider | |
Priv crypto.PrivKey |
are there cases where the publishing process won't have the private keys of the whole family?
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 thought a lot about it. In our current usecase - allow a provider to scale up by adding new bitswap aliases on other libp2p hosts - the OG provider should have access too all XP keys.
If going forwards we have such usecase when the main provider and it's XPs are in different security domains, then we can extend the protocol between two different index-provider
s to be able to send partially signed ads to each other and assemble all required signatures that way.
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 encourage checking with the providers to understand how this works with their current key management strategy - i don't think they have the keys being shared back currently, so if they're going to add a message protocol for comms to support that, we may need to jump to also supporting the partial signing
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.
Good point, noted.
cdba1f6
to
87cebe6
Compare
xproviders.AdBuilder allows to easily create, verify and sign extended provider ads that then can be published using Engine
87cebe6
to
5afd290
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.
I may be missing it, but the missing feature for me is removing extended providers. While I suspect it will be a while before we do this on an cid by cid basis, from the beginning providers should be able to turn off bitswap after they enable it. If I'm missing something here, let me know.
Hey @hannahhoward , the idea is that you can publish an ad less the provider that needs to be removed. I.e. there is no a dedicated "remove" ad, just a new ad with an updated list. Would that work for you? |
I think as long as you can do republish without the extended provider to remove, that works. |
This PR adds support for extended providers. It's a programmatic API only. CLI and configuration support will follow up if there going to be a need for those.
In particular, the PR adds
xproviders.AdBuilder
class - a builder for constructing, signing and verifying an ad that can be then published usingEngine
.