Skip to content
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

refactor!: access-client store decoupling #228

Merged
merged 14 commits into from
Dec 6, 2022
Merged

Conversation

alanshaw
Copy link
Member

@alanshaw alanshaw commented Dec 1, 2022

Goals:

  1. Decouple store from agent
  2. Simplify agent creation
  3. Agent governs data format not store
  4. Initialization of agent data, not store
  5. DRY initialization in agent, not repeated in each store impl

See: https://gist.github.com/alanshaw/ea4bd2b0ab215ade696eac1300be577d outdated

// In regular use:
const store = new StoreIndexedDB()
const data = await store.load()
const agent = data
  ? Agent.from(data, { store })
  : await Agent.create({}, { store })

// Then StoreMemory can be deleted, since the AgentData already stores all data in
// memory. It's equivalent to:
const agent = await Agent.create()

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Dec 1, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: f412a45
Status: ✅  Deploy successful!
Preview URL: https://437468a4.ucan-protocol.pages.dev
Branch Preview URL: https://refactor-access-client.ucan-protocol.pages.dev

View logs

@gobengo
Copy link
Contributor

gobengo commented Dec 2, 2022

fyi I made a draft PR that you may wanna merge into this one and it might help a little bit with resolving conflicts with main
#244

Copy link
Contributor

@hugomrdias hugomrdias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general looks good but now the user needs to choose store driver plus signer type maybe we can find a easier solution

const agentData = new AgentData(
{
meta: { name: 'agent', type: 'device', ...init.meta },
principal: init.principal ?? (await EdSigner.generate()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

who knows which signer to use for the agent principal ? With the current code its always ed but browser need rsa

Copy link
Member Author

@alanshaw alanshaw Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, w3up-client/w3ui would choose RSA. I don't think the store implementation should choose. You can put Ed25519 keys in IndexedDB. It's the environment/application that should choose this.

We should create an agent-data.browser.js which just imports agent-data.js and overrides though.

packages/access-client/src/agent.js Show resolved Hide resolved
Comment on lines 55 to 67
static async fromStore(store, options = {}) {
await store.open()
const storedData = await store.load() // { ... } or null/undefined
return storedData
? AgentData.fromExport(storedData)
: await AgentData.create(options.initialData, {
store: {
save: async (d) => {
await store.save(d)
},
},
})
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we dont have a method to test if the store was initialized do we always call this method ? if yes we gonna need to always pass on a Signer just in case its empty ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we dont have a method to test if the store was initialized do we always call this method ?

Yes, this is probably what you use 99% of the time. Essentially store.load() is the init check (which is what is happening above).

if yes we gonna need to always pass on a Signer just in case its empty ?

No because AgentData.create generates one for you. I understand that's not great if you want to use a different key type from what AgentData gives you by default...and in that situation you'd either always pass it (as you say), or do what this method is doing in your application code. I don't know how to make that a better experience other than picking a better default in the browser.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make the store.load() do store.open() ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

humm so user needs to know about:

  • Agent
  • AgentData
  • StoreDriver
  • Signer

We can leave Signer in the options for flexibility, but right now BrowserDriver kinda requires RSASigner.

can we reduce this to:

  • Agent
  • Driver
  • Signer (this would be optional, either from the driver or from the user overriding the driver)

Most uses would just need Agent and Driver, i think we can do this by just hiding AgentData inside Agent.

Copy link
Contributor

@hugomrdias hugomrdias Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const data = driver.load()

if(data) {
  agent.from(data, driver)
}else{
  agent.create(driver, { 
    principal: await EdSigner.generate() // if we want to overriding Driver.generatePrincipal() 
  })
}

something like this ?

Copy link
Member Author

@alanshaw alanshaw Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make the store.load() do store.open() ?

Yes? Just that one? What about .save()? I'm not sure on the utility of it - users shouldn't need to do it. A separate PR perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const data = driver.load()

if(data) {
  agent.from(data, driver)
}else{
  agent.create(driver, { 
    principal: await EdSigner.generate() // if we want to overriding Driver.generatePrincipal() 
  })
}

something like this ?

yes we can do that

@alanshaw alanshaw temporarily deployed to dev December 6, 2022 12:36 Inactive
@alanshaw alanshaw temporarily deployed to dev December 6, 2022 12:40 Inactive
Copy link
Contributor

@hugomrdias hugomrdias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left one inline comment.

Maybe we could change the naming store to driver and hardcode the data type instead of using generics. Avoid having /** @type {StoreConf<import('../types').AgentDataExport>} */ everywhere. These two are non blocking feel free to ignore.

packages/access-client/src/agent.js Outdated Show resolved Hide resolved
@alanshaw alanshaw changed the title refactor: access-client refactor!: access-client Dec 6, 2022
@alanshaw alanshaw changed the title refactor!: access-client refactor!: access-client store decoupling Dec 6, 2022
Copy link
Contributor

@hugomrdias hugomrdias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, love it much better

@alanshaw alanshaw temporarily deployed to dev December 6, 2022 20:17 Inactive
@alanshaw alanshaw merged commit a785278 into main Dec 6, 2022
@alanshaw alanshaw deleted the refactor/access-client branch December 6, 2022 20:19
alanshaw pushed a commit that referenced this pull request Dec 6, 2022
🤖 I have created a release *beep* *boop*
---


##
[8.0.0](access-v7.0.2...access-v8.0.0)
(2022-12-06)


### ⚠ BREAKING CHANGES

* access-client store decoupling
([#228](#228))
* upgrade to `@ucanto/{interface,principal}`@^4.0.0
([#238](#238))
* follow up on the capabilities extract
([#239](#239))

### Features

* **access-client:** cli and recover
([#207](#207))
([adb3a8d](adb3a8d))
* follow up on the capabilities extract
([#239](#239))
([ef5e779](ef5e779))
* Revert "feat!: upgrade to `@ucanto/{interface,principal}`@^4.0.0"
([#245](#245))
([c182bbe](c182bbe))
* upgrade to `@ucanto/{interface,principal}`@^4.0.0
([#238](#238))
([2f3bab8](2f3bab8))


### Bug Fixes

* connection method is not async
([#222](#222))
([0dd1633](0dd1633))


### Code Refactoring

* access-client store decoupling
([#228](#228))
([a785278](a785278))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
gobengo added a commit that referenced this pull request Dec 7, 2022
…#246)

Motivation:
* second attempt at this
#238
* after first was reverted
#245

Warning:
* the versions of ucanto we're upgrading to were backward-incompatible.
storacha/ucanto#149
* Old 'SignerArchive' exports will probably no longer import using the
new `@ucanto/principal@^4.0.0` `SignerArchive`
* in slack, @alanshaw said he thinks this is unlikely to cause problems:
"There’s no code in production that uses toArchive"

Blockers
* [x] Wait for these two to land first (@hugomrdias says so)
  * [x] @hugomrdias #207
  * [x] @alanshaw #228
* [x] then will need to resolve merge conflicts on this branch
* [x] @gobengo resolve conflicts after
#207
* [x] @gobengo resolve conflicts after
#228

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
hugomrdias added a commit that referenced this pull request Dec 13, 2022
🤖 I have created a release *beep* *boop*
---


##
[4.0.0](access-api-v3.0.0...access-api-v4.0.0)
(2022-12-13)


### ⚠ BREAKING CHANGES

* upgrade access-api @ucanto/* and @ipld/dag-ucan major versions
([#246](#246))
* access-client store decoupling
([#228](#228))
* upgrade to `@ucanto/{interface,principal}`@^4.0.0
([#238](#238))
* follow up on the capabilities extract
([#239](#239))
* doc capabilities & make requierd nb non-optionals
([#159](#159))
* Remove 0.8 caps and add account delegation to the service
([#123](#123))
* bump to 0.9
([#116](#116))

### Features

* [#153](#153)
([#177](#177))
([d6d448c](d6d448c))
* access-api uses DID env variable when building its ucanto server id
([#275](#275))
([311da78](311da78))
* **access-client:** cli and recover
([#207](#207))
([adb3a8d](adb3a8d))
* account recover with email
([#149](#149))
([6c659ba](6c659ba))
* bump to 0.9
([#116](#116))
([3e0b63f](3e0b63f))
* doc capabilities & make requierd nb non-optionals
([#159](#159))
([6496773](6496773))
* follow up on the capabilities extract
([#239](#239))
([ef5e779](ef5e779))
* Remove 0.8 caps and add account delegation to the service
([#123](#123))
([878f8c9](878f8c9)),
closes [#117](#117)
[#121](#121)
* Revert "feat!: upgrade to `@ucanto/{interface,principal}`@^4.0.0"
([#245](#245))
([c182bbe](c182bbe))
* sync encode/decode delegations
([#276](#276))
([ab981fb](ab981fb))
* upgrade access-api @ucanto/* and @ipld/dag-ucan major versions
([#246](#246))
([5e663d1](5e663d1))
* upgrade to `@ucanto/{interface,principal}`@^4.0.0
([#238](#238))
([2f3bab8](2f3bab8))


### Bug Fixes

* 0.9 ([#78](#78))
([1b1ed01](1b1ed01))
* add validation copy
([#257](#257))
([7f50af4](7f50af4)),
closes [#139](#139)
* fix Access API cannot get space/info
[#243](#243)
([#255](#255))
([1bacd54](1bacd54))
* fix d1 migrations
([#264](#264))
([fb8c09d](fb8c09d))
* make d1 spaces.metadata nullable and change to kysely
([#284](#284))
([c8a9ce5](c8a9ce5)),
closes [#280](#280)
* make multiformats 9 go away
([#133](#133))
([2668880](2668880))
* miniflare dev script
([#137](#137))
([1c5a4e2](1c5a4e2))
* testing staging deploy
([3d500fa](3d500fa))
* try to deploy api staging
([18b7b29](18b7b29))
* try to deploy staging
([c616818](c616818))


### Code Refactoring

* access-client store decoupling
([#228](#228))
([a785278](a785278))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
gobengo pushed a commit that referenced this pull request Apr 11, 2023
Goals:
1. Decouple store from agent
2. Simplify agent creation
3. Agent governs data format not store
4. Initialization of agent data, not store
5. DRY initialization in agent, not repeated in each store impl

~~See:
https://gist.github.com/alanshaw/ea4bd2b0ab215ade696eac1300be577d~~
outdated

```js
// In regular use:
const store = new StoreIndexedDB()
const data = await store.load()
const agent = data
  ? Agent.from(data, { store })
  : await Agent.create({}, { store })

// Then StoreMemory can be deleted, since the AgentData already stores all data in
// memory. It's equivalent to:
const agent = await Agent.create()
```
gobengo pushed a commit that referenced this pull request Apr 11, 2023
🤖 I have created a release *beep* *boop*
---


##
[8.0.0](access-v7.0.2...access-v8.0.0)
(2022-12-06)


### ⚠ BREAKING CHANGES

* access-client store decoupling
([#228](#228))
* upgrade to `@ucanto/{interface,principal}`@^4.0.0
([#238](#238))
* follow up on the capabilities extract
([#239](#239))

### Features

* **access-client:** cli and recover
([#207](#207))
([720dafb](720dafb))
* follow up on the capabilities extract
([#239](#239))
([717fcaa](717fcaa))
* Revert "feat!: upgrade to `@ucanto/{interface,principal}`@^4.0.0"
([#245](#245))
([197439e](197439e))
* upgrade to `@ucanto/{interface,principal}`@^4.0.0
([#238](#238))
([309aff0](309aff0))


### Bug Fixes

* connection method is not async
([#222](#222))
([7c4e0b6](7c4e0b6))


### Code Refactoring

* access-client store decoupling
([#228](#228))
([64c7496](64c7496))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
gobengo added a commit that referenced this pull request Apr 11, 2023
…#246)

Motivation:
* second attempt at this
#238
* after first was reverted
#245

Warning:
* the versions of ucanto we're upgrading to were backward-incompatible.
storacha/ucanto#149
* Old 'SignerArchive' exports will probably no longer import using the
new `@ucanto/principal@^4.0.0` `SignerArchive`
* in slack, @alanshaw said he thinks this is unlikely to cause problems:
"There’s no code in production that uses toArchive"

Blockers
* [x] Wait for these two to land first (@hugomrdias says so)
  * [x] @hugomrdias #207
  * [x] @alanshaw #228
* [x] then will need to resolve merge conflicts on this branch
* [x] @gobengo resolve conflicts after
#207
* [x] @gobengo resolve conflicts after
#228

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
gobengo pushed a commit that referenced this pull request Apr 11, 2023
🤖 I have created a release *beep* *boop*
---


##
[4.0.0](access-api-v3.0.0...access-api-v4.0.0)
(2022-12-13)


### ⚠ BREAKING CHANGES

* upgrade access-api @ucanto/* and @ipld/dag-ucan major versions
([#246](#246))
* access-client store decoupling
([#228](#228))
* upgrade to `@ucanto/{interface,principal}`@^4.0.0
([#238](#238))
* follow up on the capabilities extract
([#239](#239))
* doc capabilities & make requierd nb non-optionals
([#159](#159))
* Remove 0.8 caps and add account delegation to the service
([#123](#123))
* bump to 0.9
([#116](#116))

### Features

* [#153](#153)
([#177](#177))
([7fbf2a1](7fbf2a1))
* access-api uses DID env variable when building its ucanto server id
([#275](#275))
([e8326ec](e8326ec))
* **access-client:** cli and recover
([#207](#207))
([720dafb](720dafb))
* account recover with email
([#149](#149))
([91ad47d](91ad47d))
* bump to 0.9
([#116](#116))
([29cf63c](29cf63c))
* doc capabilities & make requierd nb non-optionals
([#159](#159))
([f6b6d06](f6b6d06))
* follow up on the capabilities extract
([#239](#239))
([717fcaa](717fcaa))
* Remove 0.8 caps and add account delegation to the service
([#123](#123))
([c3c58b9](c3c58b9)),
closes [#117](#117)
[#121](#121)
* Revert "feat!: upgrade to `@ucanto/{interface,principal}`@^4.0.0"
([#245](#245))
([197439e](197439e))
* sync encode/decode delegations
([#276](#276))
([9d48372](9d48372))
* upgrade access-api @ucanto/* and @ipld/dag-ucan major versions
([#246](#246))
([65d191c](65d191c))
* upgrade to `@ucanto/{interface,principal}`@^4.0.0
([#238](#238))
([309aff0](309aff0))


### Bug Fixes

* 0.9 ([#78](#78))
([561c68b](561c68b))
* add validation copy
([#257](#257))
([0bd49ce](0bd49ce)),
closes [#139](#139)
* fix Access API cannot get space/info
[#243](#243)
([#255](#255))
([1a74031](1a74031))
* fix d1 migrations
([#264](#264))
([5b8a6e7](5b8a6e7))
* make d1 spaces.metadata nullable and change to kysely
([#284](#284))
([7f09479](7f09479)),
closes [#280](#280)
* make multiformats 9 go away
([#133](#133))
([cdb4109](cdb4109))
* miniflare dev script
([#137](#137))
([f2cffb2](f2cffb2))
* testing staging deploy
([975cffc](975cffc))
* try to deploy api staging
([5130464](5130464))
* try to deploy staging
([4e56657](4e56657))


### Code Refactoring

* access-client store decoupling
([#228](#228))
([64c7496](64c7496))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants