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

Create a simplified kepler API. #35

Merged
merged 15 commits into from Apr 8, 2022
Merged

Create a simplified kepler API. #35

merged 15 commits into from Apr 8, 2022

Conversation

cobward
Copy link
Collaborator

@cobward cobward commented Mar 14, 2022

Based on the API described here.

Todo:

  • Currently creates a new orbit each time. Once the orbit URI work is complete we can deterministically generate the orbit id and therefore will be able to connect to the same orbit from different dapps without needing to store anything client-side.
  • Discuss and come up with appropriate types for the response from the orbit methods (or is the fetch Response type sufficient?) Sam agreed on slack that the fetch Response is appropriate, and I believe Kelsey and Charles previously agreed on this type too. See new design in the proposal above.
  • Refactor the codebase so that this is the only way to use the sdk. This can be done once the orbit URI and capability changes are done.

@cobward
Copy link
Collaborator Author

cobward commented Mar 14, 2022

When working on this I realised it would be really nice if we could delegate the "create" capability to the session key, as then the user would only need to sign one SIWE message. Is this something that will be possible with the new cap subsystem @chunningham ?

@krhoda
Copy link
Contributor

krhoda commented Mar 14, 2022

Curious about the change in target for the tsconfig, why did that need to change?

Also, this patch to the package.json like this would get all dependencies and devDependencies sorted properly:

From a299cbcddf4e50f310ce861d42288c3ef42b2670 Mon Sep 17 00:00:00 2001
From: K Rhoda <kelsey.rhoda@spruceid.com>
Date: Mon, 14 Mar 2022 09:04:15 -0600
Subject: [PATCH] Adds patch for minimal sdk deps

---
 package.json | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/package.json b/package.json
index bc859a9..73c4139 100644
--- a/package.json
+++ b/package.json
@@ -19,29 +19,29 @@
     "transform": {}
   },
   "devDependencies": {
-    "@airgap/beacon-sdk": "^2.2.3",
-    "@spruceid/didkit-wasm": "0.1.9",
-    "@spruceid/zcap-providers": "^0.1.1",
-    "@taquito/signer": "^8.1.1",
-    "@types/jest": "^26.0.22",
     "@types/mime-types": "^2.1.1",
     "@types/node": "^16.11.6",
-    "ethers": "^5.5.1",
-    "fetch-blob": "^2.1.2",
     "jest": "^26.6.3",
     "jest-localstorage-mock": "^2.4.9",
     "mime-types": "^2.1.33",
-    "siwe": "^0.1.4",
     "ts-jest": "^26.5.4",
     "ts-node": "^10.4.0",
     "typescript": "^4.2.3",
     "walkdir": "^0.4.1"
   },
   "dependencies": {
+    "@airgap/beacon-sdk": "^2.2.3",
+    "@spruceid/didkit-wasm": "0.1.9",
+    "@spruceid/zcap-providers": "^0.1.1",
+    "@taquito/signer": "^8.1.1",
+    "@types/jest": "^26.0.22",
     "cids": "^1.1.6",
     "cross-fetch": "^3.1.4",
+    "ethers": "^5.5.1",
+    "fetch-blob": "^2.1.2",
     "form-data": "^4.0.0",
     "multihashing-async": "^2.1.2",
-    "rfc4648": "^1.5.0"
+    "rfc4648": "^1.5.0",
+    "siwe": "^0.1.4"
   }
 }
--
2.35.1

@cobward
Copy link
Collaborator Author

cobward commented Mar 14, 2022

@krhoda thanks for the patch. I think I changed the target when I was testing the behaviour of something async, but there's no reason for it to be ES2017 -- I'll revert

@chunningham
Copy link
Member

it makes sense, no semantic or logical reason the peer permission can't be delegated alongside read and write

@cobward cobward marked this pull request as ready for review March 31, 2022 11:20
@cobward
Copy link
Collaborator Author

cobward commented Mar 31, 2022

it makes sense, no semantic or logical reason the peer permission can't be delegated alongside read and write

FYI - when trying to do this I couldn't work out how to construct the invocation to contain the generated peer ID. In the SIWE authn, the peer id is stored in the URI field, but I couldn't work out what the analogue for that is in the zcap authn.

@cobward cobward requested a review from sbihel March 31, 2022 11:52
@chunningham
Copy link
Member

bit of misleading code there, that message is actually a delegation, so the URI is the delegate/peer. in the zcap delegation the equivalent is invoker (but might soon change to controller)

Copy link
Member

@chunningham chunningham left a comment

Choose a reason for hiding this comment

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

Looks good to me, although I would recommend we don't restrict the mime types when putting, as I think there's no reason we should prevent users from uploading any type they want and it doesn't make it easier to use. I much enjoyed seeing how much cleaner index.ts and other stuff is though

package.json Outdated Show resolved Hide resolved
src/kepler.ts Outdated Show resolved Hide resolved
src/orbit.ts Outdated Show resolved Hide resolved
src/orbit.ts Outdated Show resolved Hide resolved
src/kepler.ts Outdated Show resolved Hide resolved
src/util.ts Outdated Show resolved Hide resolved
src/index.test.ts Outdated Show resolved Hide resolved
@cobward
Copy link
Collaborator Author

cobward commented Mar 31, 2022

I would recommend we don't restrict the mime types when putting, as I think there's no reason we should prevent users from uploading any type they want and it doesn't make it easier to use.

Spoke about this in the Kepler meeting. To clarify, the put function doesn't restrict mime types, it just only knows how to construct blobs for certain types:

  • If you do orbit.put(key, value, { type: "text/<text-type>"}), then the sdk will store a new Blob([value], {type: 'text/<text-type>'}).
  • If you do orbit.put(key, value, { type: "application/json"}), then the sdk will store a new Blob([JSON.serialize(value)], {type: 'application/json'}.
  • If you do orbit.put(key, value), and value.constructor.name === 'Blob', then the sdk will just store that blob.
  • Otherwise the sdk will error as it does not understand how to store the value you have provided.

This gives us the flexibility to support other types directly in the future, but for now users would need to construct a Blob themselves if they want to store something that isn't text or json.

An alternative would be to remove the type property from the request options, and instead provide 3 put functions, (i.e. putText, putJson, putBlob). IMO this would make the code simpler and more obvious at the expense of a slightly broader API, so if there is an appetite for that I would happily switch.

Another alternative would be to again remove the type property from the request options, and instead try and guess what type the value is. The best we could do is:

  • store values of type string as text/plain.
  • store values with value.constructor.name === 'Blob' directly.
  • try to json-serialise anything else and store it as application/json.

I'm not a massive fan of this since json-serialisation fails silently, so a user might not know that some of the data is lost because it could not be parsed. But it would give us a simple api (everything is orbit.put(key, value).

Opinions? @wyc @skgbafa @chunningham

@cobward
Copy link
Collaborator Author

cobward commented Mar 31, 2022

For @skgbafa and anyone else who wants to test this, this branch is currently dependent on the fat_packaged version of didkit, which can be built by running this script:
https://github.com/spruceid/didkit/blob/feat/fat_bundle/lib/web/fat_bundle.sh

Copy link
Member

@chunningham chunningham left a comment

Choose a reason for hiding this comment

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

mis-read the put mime-type logic, LGTM

package.json Outdated Show resolved Hide resolved
Also fixed a bug where File's couldn't be put directly.
When testing building a test dapp I found some improvements that could be
made to reduce polyfilling.
@cobward
Copy link
Collaborator Author

cobward commented Apr 6, 2022

After more testing and discussions with Simon, we decided the best option would be to vendor the didkit fat pkg here until we figure out and resolve the async import issue.

Copy link

@skgbafa skgbafa left a comment

Choose a reason for hiding this comment

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

  • Overall the PR looks great! Love the tsdoc. Only reviewed code, did not execute functions.
  • Some small js/stucture feedback
    • Do we see more changes possible for config stuff? If so, setting up a global config pattern could be good. Not worth doing if we don't see much change
      • invHeaderStr
      • "https://kepler.test.spruceid.xyz:443"
    • Add a linter? Maybe later. For collaboration reasons.
    • Add .nvmrc. For collaboration reasons.
  • Deploy the docs to gh pages? Add a small script to do so? Maybe done by a gh action as part of a release process?

Not running tests yet since that will need us to spin up a Kepler
instance. Will set that up once we're at a stable point (probably after
the HTTP API changes).
@cobward cobward force-pushed the feat/minimal-sdk branch 2 times, most recently from ac6c81c to a38c628 Compare April 8, 2022 11:07
@cobward
Copy link
Collaborator Author

cobward commented Apr 8, 2022

Linter and .nvmrc added. Will address the publishing of the docs in a future PR.

@cobward cobward merged commit 25f13e7 into main Apr 8, 2022
@cobward cobward deleted the feat/minimal-sdk branch April 8, 2022 11:11
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.

None yet

4 participants