-
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: access-api handles provider/add invocations #462
Conversation
5498b9f
to
2f64d27
Compare
@Gozala I made a few changes based on your suggestions, and linked to the commits under your feedback comments. |
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.
Can you just add cid
field to the table and otherwise I think it's all good.
-- provision: the action of providing or supplying something for use | ||
-- use case: representing the registration of a storage provider to a space | ||
IF NOT EXISTS provisions ( | ||
id INTEGER NOT NULL PRIMARY KEY, |
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.
Can we still add the cid
field per our discussion in slack. We can drop it later if we decide it's not needed, but I think we'll want to tie these to the invocations that provisioned the provider. If we don't write those now it would be really hard to trace them later.
.selectFrom(this.tableNames.provisions) | ||
.select((e) => e.fn.count('provider').as('size')) | ||
.executeTakeFirstOrThrow() | ||
return BigInt(size) |
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 it otherwise a string ? Can you plz add a comment explaining why this needs to be a BigInt.
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 made it a BigInt to match the interface. I used bigint in the interface for consistency with the count()
method on delegations, which I made return a BigInt per @alanshaw review. #427 (comment)
.values(rows) | ||
.executeTakeFirstOrThrow() | ||
} | ||
|
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.
By subscriber I assume you mean the account DIDs?
Yes
CREATE TABLE | ||
-- provision: the action of providing or supplying something for use | ||
-- use case: representing the registration of a storage provider to a space | ||
IF NOT EXISTS provisions ( |
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: In the past when I had to do more DB work using plural names for tables was considered a bad practice, stack overflow still seems to think the singular is a the way to go with bunch of reasons there.
I'm not going to get upset if we use plural, but might be worth considering singular names instead.
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.
in general I don't mind singular at all, and maybe even slightly prefer it, but here I made it plural because delegations
accounts
spaces
already was.
I don't have any objection to batch changing them to singular in one big other PR
/** | ||
* action which results in provisionment of a space consuming a storage provider | ||
*/ | ||
export interface StorageProvisionCreation { |
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 find naming to be very confusing, can we simply call this
export interface StorageProvisionCreation { | |
export interface ProvisionModel { |
Or better yet
export interface StorageProvisionCreation { | |
export interface Provision { |
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.
/** | ||
* stores instances of a storage provider being consumed by a consumer | ||
*/ | ||
export interface Provisions { |
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 also find it more intuitive if it was called following instead
export interface Provisions { | |
export interface ProvisionStore { |
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 called Delegations DelegationsStorage
. When I did alan commented that it didn't look like the other types that were called e.g. Delegations
. I'm not against changing these names in subsequent PRs. If we call it ProvisionStore we should also change DelegationsStorage to DelegationsStore IMO
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.
renamed to ProvisionsStorage and idc if we rename to something else after. 9b9a14d
(My original intention was to eventually have like type Provision = StorageProvision | OtherKindOfProvision
)
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 agree with @Gozala but I want consistency in the code base. Can you please open another PR to bring the others in line?
const account = { did: () => accountDID } | ||
const space = await principal.ed25519.generate() | ||
const service = await providerAddHandlerVariant.audience | ||
const agentACanSignAsAccountDID = await ucanto.delegate({ |
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'm not sure why this is passing, but ucanto no longer knows anything about ./update
. In fact now that you've pointed out it is passing tests, I'm concerned that it does as I would expect it to fail instead.
🤖 I have created a release *beep* *boop* --- ## [3.1.0](capabilities-v3.0.0...capabilities-v3.1.0) (2023-03-04) ### Features * access-api handles provider/add invocations ([#462](#462)) ([5fb56f7](5fb56f7)) * includes proofs chains in the delegated authorization chain ([#467](#467)) ([5144293](5144293)) --- 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* --- ## [10.0.0](access-v9.4.0...access-v10.0.0) (2023-03-08) ### ⚠ BREAKING CHANGES * 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)) * handle access/delegate invocations without error ([#427](#427)) ([4f0bd1c](4f0bd1c)) * upgrade capabilities to latest ucanto ([#463](#463)) ([2d786ee](2d786ee)) * upgrade to new ucanto ([#498](#498)) ([dcb41a9](dcb41a9)) ### Bug Fixes * allow injecting email ([#466](#466)) ([e19847f](e19847f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). 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).
Motivation: * implement #459 * handle `provider/add` as a way of registering space Steps * [x] scaffold with in-memory data store * [x] make it work with sqlite data store --------- Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
🤖 I have created a release *beep* *boop* --- ## [3.1.0](capabilities-v3.0.0...capabilities-v3.1.0) (2023-03-04) ### Features * access-api handles provider/add invocations ([#462](#462)) ([46da0df](46da0df)) * includes proofs chains in the delegated authorization chain ([#467](#467)) ([743a72f](743a72f)) --- 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* --- ## [10.0.0](access-v9.4.0...access-v10.0.0) (2023-03-08) ### ⚠ BREAKING CHANGES * 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)) * handle access/delegate invocations without error ([#427](#427)) ([db01d07](db01d07)) * upgrade capabilities to latest ucanto ([#463](#463)) ([e375ae4](e375ae4)) * upgrade to new ucanto ([#498](#498)) ([790750d](790750d)) ### Bug Fixes * allow injecting email ([#466](#466)) ([b4b0173](b4b0173)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). 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)) ([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).
Motivation:
provider/add
as a way of registering spaceSteps