-
Notifications
You must be signed in to change notification settings - Fork 22
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: allow multiple providers #595
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.
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.
Looks great! But I think we can't land this until we impose constraint that only one of the providers can be added to the space. I provided more detailed comment inline.
My lest urgent concern is that we (as in web3.storage) decides who gets to have a nft.storage provider as opposed to nft.storage itself. It's probably fine if we want to land and unblock work on the client side, but I do think we need to address this sooner than later.
/** | ||
* action which results in provisionment of a space consuming a storage provider | ||
*/ | ||
export interface Provision<ServiceDID extends Ucanto.DID<'web'>> { | ||
invocation: Ucanto.Invocation<ProviderAdd> | ||
space: Ucanto.DID<'key'> | ||
account: Ucanto.DID<'mailto'> | ||
provider: AlphaStorageProvider | ServiceDID | ||
provider: ServiceDID |
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.
Nit: I would personally loose the generic and use Ucanto.DID<'web'>
here. Which I think would make ts-expect-error in file above go away as well.
} | ||
|
||
/** | ||
* stores instances of a storage provider being consumed by a consumer | ||
*/ | ||
export interface ProvisionsStorage<ServiceDID extends Ucanto.DID<'web'>> { | ||
service: ServiceDID | ||
services: ServiceDID[] |
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.
Nit: I'd remove these from the interface entirely. When I was designing provider protocol I was assuming that we would have provider/publish
capability which would allow you to create a provider and consequently allow others to add it if they received consumer/add
delegation from you. I realize we're far from there, but it still might be good idea to align on it. Implementation could still have such a field.
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 did start out that way but it's a bigger change set and neither of you were awake for me to double check with ;)
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.
As far as I can tell there are no constraints imposed at the DB layer that would prevent one from adding both nft.storage and web3.storage providers to the same space
We should impose that constraint at some layer to prevent having spaces with both providers otherwise we'll run into problem where user might add something to a space and we don't know if it's an nft or web3 content.
}, | ||
])) { | ||
describe(`provider/add ${providerAddHandlerVariant.name}`, () => { | ||
it(`can be invoked by did:key`, async () => { |
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.
Note: This test was removed because it was not testing a right thing and was only passing because it did not used validator.
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 have removed bunch of indirection in the tests because those loops with test variants made my head heart and I could not get my head around what what was passed where. Instead I've added some test utility functions so each test can setup whatever it needs test and then test.
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
Motivation: * make it easier to merge #595 --------- Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
🤖 I have created a release *beep* *boop* --- ## [5.0.0](access-api-v4.11.0...access-api-v5.0.0) (2023-03-23) ### ⚠ BREAKING CHANGES * implement new account-based multi-device flow ([#433](#433)) * upgrade capabilities to latest ucanto ([#463](#463)) ### Features * access-api handles provider/add invocations ([#462](#462)) ([5fb56f7](5fb56f7)) * access-api serves access/claim invocations ([#456](#456)) ([baacf35](baacf35)) * access/authorize confirmation email click results in a delegation back to the issuer did:key so that access/claim works ([#460](#460)) ([a466a7d](a466a7d)) * allow multiple providers ([#595](#595)) ([96c5a2e](96c5a2e)) * define `access/confirm` handler and use it in ucanto-test-utils registerSpaces + validate-email handler ([#530](#530)) ([b1bbc90](b1bbc90)) * handle access/delegate invocations without error ([#427](#427)) ([4f0bd1c](4f0bd1c)) * if POST /validate-email?mode=authorize catches error w/ too big qr code ([#516](#516)) ([d0df525](d0df525)) * implement new account-based multi-device flow ([#433](#433)) ([1ddc6a0](1ddc6a0)) * includes proofs chains in the delegated authorization chain ([#467](#467)) ([5144293](5144293)) * move access-api delegation bytes out of d1 and into r2 ([#578](#578)) ([4510c9a](4510c9a)) * move validation flow to a Durable Object to make it ⏩ fast ⏩ fast ⏩ fast ⏩ ([#449](#449)) ([02d7552](02d7552)) * provision provider type is now the DID of the w3s service ([#528](#528)) ([6a72855](6a72855)) * space/info will not error for spaces that have had storage provider added via provider/add ([#510](#510)) ([ea4e872](ea4e872)) * upgrade capabilities to latest ucanto ([#463](#463)) ([2d786ee](2d786ee)) * upgrade to new ucanto ([#498](#498)) ([dcb41a9](dcb41a9)) * write invocations and receipts into ucan log ([#592](#592)) ([754bf52](754bf52)) ### Bug Fixes * access/delegate checks hasStorageProvider(space) in a way that provider/add allows access/delegate ([#483](#483)) ([f4c640d](f4c640d)) * adjust migration 0005 to keep delegations table but create new used delegations_v2 ([#469](#469)) ([a205ad1](a205ad1)) * adjust migration 0005 to not do a drop table and instead rename delegations -> delegations_old and create a new delegations ([#468](#468)) ([6c8242d](6c8242d)) * allow injecting email ([#466](#466)) ([e19847f](e19847f)) * DbDelegationsStorage#find throws UnexpectedDelegation w/ { row } if failed bytesToDelegations ([#476](#476)) ([a6dafcb](a6dafcb)) * DbProvisionsStorage putMany doesnt error on cid col conflict ([#517](#517)) ([c1fea63](c1fea63)) * delegations model tries to handle if row.bytes is Array not Buffer (e.g. cloudflare) ([#478](#478)) ([030e7b7](030e7b7)) ### Miscellaneous Chores * **access-client:** release 11.0.0-rc.0 ([#573](#573)) ([be4386d](be4386d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- ## [4.0.1](capabilities-v4.0.0...capabilities-v4.0.1) (2023-03-27) ### Features * allow multiple providers ([#595](#595)) ([96c5a2e](96c5a2e)) ### Miscellaneous Chores * **access-client:** release 11.0.0-rc.0 ([#573](#573)) ([be4386d](be4386d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Allows multiple providers to be used with the system and defines web3 and NFT providers in config. --------- Co-authored-by: Irakli Gozalishvili <contact@gozala.io> Co-authored-by: Benjamin Goering <171782+gobengo@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ## [5.0.0](access-api-v4.11.0...access-api-v5.0.0) (2023-03-23) ### ⚠ BREAKING CHANGES * implement new account-based multi-device flow ([#433](#433)) * upgrade capabilities to latest ucanto ([#463](#463)) ### Features * access-api handles provider/add invocations ([#462](#462)) ([46da0df](46da0df)) * access-api serves access/claim invocations ([#456](#456)) ([2ec16e9](2ec16e9)) * access/authorize confirmation email click results in a delegation back to the issuer did:key so that access/claim works ([#460](#460)) ([fc62691](fc62691)) * allow multiple providers ([#595](#595)) ([aba57b3](aba57b3)) * define `access/confirm` handler and use it in ucanto-test-utils registerSpaces + validate-email handler ([#530](#530)) ([a08b513](a08b513)) * handle access/delegate invocations without error ([#427](#427)) ([db01d07](db01d07)) * if POST /validate-email?mode=authorize catches error w/ too big qr code ([#516](#516)) ([ab83b19](ab83b19)) * implement new account-based multi-device flow ([#433](#433)) ([6152e55](6152e55)) * includes proofs chains in the delegated authorization chain ([#467](#467)) ([743a72f](743a72f)) * move access-api delegation bytes out of d1 and into r2 ([#578](#578)) ([3029e4a](3029e4a)) * move validation flow to a Durable Object to make it ⏩ fast ⏩ fast ⏩ fast ⏩ ([#449](#449)) ([3868d97](3868d97)) * provision provider type is now the DID of the w3s service ([#528](#528)) ([4cd6cd9](4cd6cd9)) * space/info will not error for spaces that have had storage provider added via provider/add ([#510](#510)) ([362024f](362024f)) * upgrade capabilities to latest ucanto ([#463](#463)) ([e375ae4](e375ae4)) * upgrade to new ucanto ([#498](#498)) ([790750d](790750d)) * write invocations and receipts into ucan log ([#592](#592)) ([d52a281](d52a281)) ### Bug Fixes * access/delegate checks hasStorageProvider(space) in a way that provider/add allows access/delegate ([#483](#483)) ([1d3d562](1d3d562)) * adjust migration 0005 to keep delegations table but create new used delegations_v2 ([#469](#469)) ([d90825a](d90825a)) * adjust migration 0005 to not do a drop table and instead rename delegations -> delegations_old and create a new delegations ([#468](#468)) ([89f2acd](89f2acd)) * allow injecting email ([#466](#466)) ([b4b0173](b4b0173)) * DbDelegationsStorage#find throws UnexpectedDelegation w/ { row } if failed bytesToDelegations ([#476](#476)) ([660f773](660f773)) * DbProvisionsStorage putMany doesnt error on cid col conflict ([#517](#517)) ([8c6dea8](8c6dea8)) * delegations model tries to handle if row.bytes is Array not Buffer (e.g. cloudflare) ([#478](#478)) ([02c0c28](02c0c28)) ### Miscellaneous Chores * **access-client:** release 11.0.0-rc.0 ([#573](#573)) ([29daa02](29daa02)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- ## [4.0.1](capabilities-v4.0.0...capabilities-v4.0.1) (2023-03-27) ### Features * allow multiple providers ([#595](#595)) ([aba57b3](aba57b3)) ### Miscellaneous Chores * **access-client:** release 11.0.0-rc.0 ([#573](#573)) ([29daa02](29daa02)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Allows multiple providers to be used with the system and defines web3 and NFT providers in config.