feat: w3name #648

alanshaw · 2021-11-17T15:35:05Z

This PR implements w3name as specified in https://github.com/alanshaw/w3name with some changes to the DB schema because it turns out IPNS record newness isn't solely determined by sequence number.

Demo:

vasco-santos

Nice, a lot of great things here! 🚀 ⚡

vasco-santos · 2021-11-19T11:31:40Z

packages/db/index.js

+      throw new DBError(error)
+    }
+
+    return data.length ? data[0].record : undefined


Why not using .single() in the supabase client req?

I believe it throws if not found (and I couldn't be bothered to figure out the particular error code it uses) and I want to to return undefined if not found.

yeah, it will return an error and thrown with the error on line 750
If this is not what we want, we should rethink some other methods like getKey, getUpload

In general I'm anti throw when things are not found at the data storage layer. In JS it's idiomatic for things to return undefined like with Map, Set,Array etc. also with regular Objects.

I also wanted to return a better error message than what we'd get from the DB.

Also, we should do this for the other methods because I just saw this in sentry:

Yes, that's the getKey that I was mentioning. Can you open an issue there for us to change it?

I also prefer to return undefined here

Hey, I agree while working on the pinning APIs I've encountered the same problem.
I think in general we should look into abstracting the logic to interpret different DB errors into smaller and more defined ones (ie not found, foreign key violations, connection error etc). Because as you mentioned at the moment everything is caught and on the API level we would send different errors based on the actual returns.

packages/db/postgres/functions.sql

vasco-santos · 2021-11-19T11:39:14Z

packages/db/postgres/functions.sql

+        -- https://github.com/ipfs/go-ipns/blob/a8379aa25ef287ffab7c5b89bfaad622da7e976d/ipns.go#L325
+        ((data ->> 'has_v2_sig')::BOOLEAN = TRUE AND name.has_v2_sig = FALSE) OR
+        ((data ->> 'has_v2_sig')::BOOLEAN = name.has_v2_sig AND (data ->> 'seqno')::BIGINT > name.seqno) OR
+        ((data ->> 'has_v2_sig')::BOOLEAN = name.has_v2_sig AND (data ->> 'seqno')::BIGINT = name.seqno AND (data ->> 'validity')::BIGINT > name.validity) OR


No new records should be created with same seqno. Only if a bug exists right?

Not sure if I follow, in https://github.com/ipfs/go-ipns/blob/a8379aa25ef287ffab7c5b89bfaad622da7e976d/ipns.go#L325 a new record will be used if the seqno's are both the same and the sigs are both the same but the validity of the new record is greater than the existing one.

That is odd. No new records should be created with the same seq number as far as I understand.

I would like to know why this is a possibility, but I guess we can follow what go does for now

You implemented the same here: https://github.com/ipfs/js-ipns/blob/master/src/index.js#L481-L503

I think it's ok to be stricter here and disallow overwriting same revision even if IPNS allows you to do that, unless there is a good reason to do so.

From the discussion above it appears we do not have a clear understanding of the motivation here. So I suggest we disallow that because client intentionally does the increment.

Or identify a clear reason why support this and:

Add comment stating that reason.

Consider supporting this from the client

Otherwise I fear we are misleading users by providing client API that leaves an impression of needing to increment but than can catch them by surprise of force update.

P.S.: If I had to guess, overwrite is inspired by git push --force

If we're accepting IPNS records, then our rules to update our cache should be the same as in IPFS because:

There's not anything stopping people from crafting their own IPNS records and using validity time not sequence to manage updates. So our server code needs to be prepared for that...or we need to lock the server down further to only accept records that increment the sequence number but...

My prediction is that these records might not always come through the Web3.Storage API. If we start accepting record updates over pubsub, then I want the behaviour to be the same as in IPFS.

I want Web3.Storage to have the same behaviour as IPFS. I don't want to find a bug in the far future where we're not updating our cache because we decided not to do the same thing as IPFS. One of the biggest parts of this is that it's compatible with IPFS, when we deviate from that it starts to look like a second class citizen. I'm not trying to build a better IPNS, I'm just trying to make a bridge to what's already there that people can use.

Those are fare points! Yet, I would still invite designing practical and intuitive system that can help us improve impractical one instead of constraining design by it.

I consider IPNS impractical because as far as I know no one has adopted it due to it's impracticality (or at least that is my impression).

It could be (and is likely) that there are good reasons for IPNS to do things this way & it's impracticality has other causes. If so we should probably better reflect that in the client API as well. In my experience the it is the worst when API imposes set of invariants, but then you discover that somehow those invarians weren't upheld (reminds me of fauna unique index).

vasco-santos · 2021-11-19T11:41:11Z

packages/db/postgres/functions.sql

+        ((data ->> 'has_v2_sig')::BOOLEAN = TRUE AND name.has_v2_sig = FALSE) OR
+        ((data ->> 'has_v2_sig')::BOOLEAN = name.has_v2_sig AND (data ->> 'seqno')::BIGINT > name.seqno) OR
+        ((data ->> 'has_v2_sig')::BOOLEAN = name.has_v2_sig AND (data ->> 'seqno')::BIGINT = name.seqno AND (data ->> 'validity')::BIGINT > name.validity) OR
+        ((data ->> 'has_v2_sig')::BOOLEAN = name.has_v2_sig AND (data ->> 'seqno')::BIGINT = name.seqno AND (data ->> 'validity')::BIGINT = name.validity AND DECODE(data ->> 'record', 'base64') > DECODE(name.record, 'base64'));


I could not understand why this. Can you let me know the reason for this?

This function determines if a new record is newer than another: https://github.com/ipfs/go-ipns/blob/a8379aa25ef287ffab7c5b89bfaad622da7e976d/ipns.go#L325

The code here attempts to replicate that in SQL.

I mean this bit DECODE(data ->> 'record', 'base64') > DECODE(name.record, 'base64'))

That's this bit: https://github.com/ipfs/go-ipns/blob/a8379aa25ef287ffab7c5b89bfaad622da7e976d/ipns.go#L321-L324

I see, it is not in the code, so perhaps responsibility of the caller. I don't know what is the reason for that, we don't do this at all in JS afaik, nor is in the spec

Yes, it's responsibility of the caller. https://github.com/ipfs/go-ipns/blob/5976a80227cc5199414119585cca347bc814647a/record.go#L113-L118

I couldn't find in JS either, maybe an oversight, or it's new code on the Go side?

I think it would be a good idea here to figure out a reason and add a code comment stating it. Even better if same could be done in go code & spec otherwise over time intentions can get muddy as code bases change.

packages/api/test/scripts/helpers.js

vasco-santos · 2021-11-19T11:55:11Z

packages/client/README.md

@@ -37,6 +37,40 @@ for (const file of files) {
 }
 ```

+### Mutability
+
+Mutability in Web3.Storage is maintained through IPNS records.


In line 14, we use web3.storage, we should be consistent and use the same everywhere

I can lower it here for consistency but I think we decided when written it should be Web3.Storage.

mind checking and change the other ones in this file if we want to go with Web3.Storage naming?

vasco-santos · 2021-11-19T11:55:49Z

packages/client/README.md

+
+Mutability in Web3.Storage is maintained through IPNS records.
+
+⚠️ Name records are not _yet_ published to or updated from the IPFS network. Working with name records simply updates the Web3.Storage cache of data.


Worth pointing this to an issue where people can subscribe for developments

vasco-santos · 2021-11-19T12:02:17Z

packages/client/examples/node.js/mutability.js

+  console.log('✅ Done\n')
+
+  console.log('⏳ Resolving current value...')
+  const { value: curValue, record: curRecord } = await Name.resolve(client, id)


I like to have Name exported as an util. But, I would expect resolve and publish to be part of the main client API. What are your thoughts here? I am with mixed feelings about this.

(+) It is good that it is isolated because people will need to use Name.keypair() and Name.create() to publish.

(+) We can probably isolate in the context of a shared dotstorage lib. But we should make a design session on how we want to have this lib

(-) Passing the client into Name.resolve() and Name.publish() is strange IMHO

(-) Folks can resolve by receiving an ID from a third party, having to import Name for it

I'd be happy to add publish and resolve to the main client as sugar for calling Name.publish and Name.resolve.

I think it would really help to refine the terms. Here is what would make most sense to me:

// Creates new writable name. declare function create():Promise<WritableName> // Creates writable name from keypair. declare function from(keypair:KeyPair):Promise<WritableName> // Parses string name to readable name declare function parse(name:string):Name export interface Name { // binary representation of the identifier (hash digest of the public key) bytes: Uint8Array // returns string representation of the name toString(base?:BaseEncoder):string } // Writable name is just a name with a signing key attached so it could be // used for publishing export interface WritableName extends Name { key: SigningKey } export interface SigningKey { sign(input:Uint8Array):Promise<Uint8Array> } // Creates initial revision for the given name name. Note this does not require // signing key because it is ok to create revision without it, publishing it would // require key however. declare function v0(name:Name, value:string):Revision // Creates an incremented revision declare function increment(revision:Revision, value:string):Revision interface Revision { name: Name sequence: number value: string } // Publishes given revision to a service. Requires signing key declare function publish(service:Service, revision, key:SigningKey}):Promise<Revision> // Resolves last published revision from the service declare function resolve(service:Service, name:Name):Promise<Revision>

I've implemented this as designed above. I think it's a great idea to separate the name from the record. I've also separated this from the main bundle, so you use it like import * as Name from 'web3.storage/name'. This allows us to publish it as is and try it out without effecting the main library. Once we're happy with it and we've removed libp2p-crypto to minimise the bundle size we can add sugar to the main library.

packages/client/src/name.js

Co-authored-by: Vasco Santos <santos.vasco10@gmail.com>

Gozala

I made bunch of suggestions, take it or leave it up to you. I do think Name could and probably should be a standalone library pulled in here. That would encourage "naming service" similar to "pinning service" as opposed to making it *.storage specific.

Gozala · 2021-11-22T21:26:15Z

packages/api/src/index.js

@@ -43,6 +44,9 @@ router.put('/car/:cid',             mode['📝'](auth['🔒'](carPut)))
 router.post('/upload',              mode['📝'](auth['🔒'](uploadPost)))
 router.get('/user/uploads',         mode['👀'](auth['🔒'](userUploadsGet)))

+router.get('/name/:key',            mode['👀'](auth['🤲'](nameGet)))


❓Do we need to auth ? Seems like this feature would useful in so many contexts without this.

It's not strictly needed, in the spec we create it'll be optional. However using the auth key allows us to rate limit and optionally associate records with users (which we don't do in this MVP) for metrics.

packages/api/src/name.js

packages/client/src/name.js

Gozala · 2021-11-23T14:25:29Z

packages/db/postgres/functions.sql

+          (data ->> 'has_v2_sig')::BOOLEAN,
+          (data ->> 'seqno')::BIGINT,
+          (data ->> 'validity')::BIGINT)
+  ON CONFLICT (key) DO UPDATE


I wonder if it would be a better idea to have a name table that retains all revisions with a materialized view for current revisions instead. It is really helpful in other systems and I imagine would be the case here as well.

Would also probably have better write speed (at expense of read).

Yes probably, we should move to something like that if this becomes successful.

Gozala · 2021-11-23T14:30:39Z

packages/client/src/name.js

+import { identity } from 'multiformats/hashes/identity'
+import { base36 } from 'multiformats/bases/base36'
+import { CID } from 'multiformats/cid'
+import { keys } from 'libp2p-crypto'


Do we really want to bring all this along ? Unless things has improved since this was one of the largest piece of js-ipfs.

It's on the list of things: #659

packages/client/examples/node.js/mutability.js

Gozala · 2021-11-23T15:31:58Z

packages/client/examples/node.js/mutability.js

+  console.log('✅ Done\n')
+
+  console.log('⏳ Resolving current value...')
+  const { value: curValue, record: curRecord } = await Name.resolve(client, id)


I think it would really help to refine the terms. Here is what would make most sense to me:

// Creates new writable name. declare function create():Promise<WritableName> // Creates writable name from keypair. declare function from(keypair:KeyPair):Promise<WritableName> // Parses string name to readable name declare function parse(name:string):Name export interface Name { // binary representation of the identifier (hash digest of the public key) bytes: Uint8Array // returns string representation of the name toString(base?:BaseEncoder):string } // Writable name is just a name with a signing key attached so it could be // used for publishing export interface WritableName extends Name { key: SigningKey } export interface SigningKey { sign(input:Uint8Array):Promise<Uint8Array> } // Creates initial revision for the given name name. Note this does not require // signing key because it is ok to create revision without it, publishing it would // require key however. declare function v0(name:Name, value:string):Revision // Creates an incremented revision declare function increment(revision:Revision, value:string):Revision interface Revision { name: Name sequence: number value: string } // Publishes given revision to a service. Requires signing key declare function publish(service:Service, revision, key:SigningKey}):Promise<Revision> // Resolves last published revision from the service declare function resolve(service:Service, name:Name):Promise<Revision>

Gozala · 2021-11-24T18:50:34Z

@alanshaw one other thing that had been on my mind that I have not captured anywhere, I think it would be a good idea to have an example illustrating how to use this across multiple sessions, because:

User needs to store keypair somewhere
User needs to store revision somewhere
User need to restore name from keypair. Proposed Name.from(keypair) was meant to do it.
Use needs to restore revision from somewhere. Given it's simple JSON structure it should be easy enough yet example is probably a good idea.

alanshaw · 2021-11-25T00:02:35Z

@alanshaw one other thing that had been on my mind that I have not captured anywhere, I think it would be a good idea to have an example illustrating how to use this across multiple sessions, because:

User needs to store keypair somewhere

User need to restore name from keypair. Proposed Name.from(keypair) was meant to do it.

https://github.com/web3-storage/web3.storage/blob/817bb5879bc042b201a36bc3f0b42e48b86359c6/packages/client/README.md#signing-key-management

User needs to store revision somewhere

Use needs to restore revision from somewhere. Given it's simple JSON structure it should be easy enough yet example is probably a good idea.

https://github.com/web3-storage/web3.storage/blob/817bb5879bc042b201a36bc3f0b42e48b86359c6/packages/client/README.md#revision-serializationdeserialization

alanshaw · 2021-11-25T15:56:58Z

I'm going to get this merged now. I believe I've addressed the majority of the feedback. If there's anything missed then please shout.

alanshaw requested review from vasco-santos, Gozala and hugomrdias November 18, 2021 13:47

alanshaw marked this pull request as ready for review November 18, 2021 13:47

alanshaw mentioned this pull request Nov 19, 2021

Simple Mutability with IPNS records #659

Open

7 tasks

vasco-santos requested changes Nov 19, 2021

View reviewed changes

Alan Shaw and others added 10 commits November 23, 2021 12:24

fix: script startup timed out

a5b4124

fix: revert commented code

90e89a9

feat: wip /name endpoints

fb8710f

feat: db client and db table schema

27d023e

test: add tests

9ce7800

feat: client

c290bae

fix: make work

c4ac207

fix: wip fix record update comparisons

1432154

fix: record comparisons

9033ce1

fix: remove unnecessary comment

11690bd

Co-authored-by: Vasco Santos <santos.vasco10@gmail.com>

alanshaw force-pushed the feat/w3name branch from c50d856 to 11690bd Compare November 23, 2021 12:42

Alan Shaw added 2 commits November 23, 2021 13:28

Merge branch 'fix/script-startup-timeout' into feat/w3name

62fbfca

fix: trim crypto

ae07fd0

Gozala approved these changes Nov 23, 2021

View reviewed changes

refactor: constant for cf alg

32b09c7

alanshaw mentioned this pull request Nov 23, 2021

DB client throws on 404, change to return undefined #687

Closed

Alan Shaw added 2 commits November 24, 2021 14:11

refactor: client API

35bc42e

fix: make it work again

d70c645

Alan Shaw added 2 commits November 24, 2021 22:59

feat: separate from main lib temporarily, add docs

e0bf47c

docs: revision serde

817bb58

Alan Shaw added 2 commits November 25, 2021 00:04

fix: find a better way to do this

b2a78ad

fix: types

cd0be71

Alan Shaw added 15 commits November 25, 2021 10:12

docs: more docs

efbc865

fix: tests

5936852

fix: remove debug flag

01e0759

fix: improve not found error message

a392845

fix: appease linter

a5d0475

fix: simplify helper

de705c4

docs: add HTTP API documentation

62e5ff6

fix: improve error message

42cd9c7

fix: tests

7ae737b

fix: really fix the tests

8c75f6a

fix: ok really really fix

691aebb

test: add first name test

2a47e32

fix: remove .only

1397e3d

test: hundreds

80e343d

Merge branch 'main' into feat/w3name

1fd8e4f

alanshaw merged commit 9c287bb into main Nov 25, 2021

alanshaw deleted the feat/w3name branch November 25, 2021 16:00

github-actions bot mentioned this pull request Jan 26, 2022

chore(main): release client 1.0.0 #908

Closed

github-actions bot mentioned this pull request Jun 27, 2022

chore(main): release db 1.0.0 #1578

Closed


		Mutability in Web3.Storage is maintained through IPNS records.

		⚠️ Name records are not _yet_ published to or updated from the IPFS network. Working with name records simply updates the Web3.Storage cache of data.

feat: w3name #648

feat: w3name #648

Conversation

alanshaw commented Nov 17, 2021 • edited Loading

vasco-santos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

alanshaw Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gozala Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanshaw Nov 25, 2021 • edited Loading

Choose a reason for hiding this comment

Gozala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gozala commented Nov 24, 2021

alanshaw commented Nov 25, 2021 • edited Loading

alanshaw commented Nov 25, 2021

alanshaw commented Nov 17, 2021 •

edited

Loading

vasco-santos Nov 19, 2021 •

edited

Loading

alanshaw Nov 23, 2021 •

edited

Loading

Gozala Nov 23, 2021 •

edited

Loading

vasco-santos Nov 19, 2021 •

edited

Loading

vasco-santos Nov 19, 2021 •

edited

Loading

alanshaw Nov 25, 2021 •

edited

Loading

alanshaw commented Nov 25, 2021 •

edited

Loading