Use CIDs for content addressing rather than plain hashes #21

snowteamer · 2023-08-06T23:59:58Z

Closes #19
Closes #20

Summary of changes

Content addressing

chel hash now outputs content identifiers (CIDs) rather than unadorned hashes. For now, CIDs created by the command include the raw (0x55) code by default, and the json (0x0200) code for JSON files.
It should now be much easier to switch the hashing algo or hash encoding, by updating constants.
Improved chel hash help message.

Testing (related to #11)

Added tests for chel hash.
Added tests for getBackend().
New deno task test command, to run the test suite with suitable CLI flags.
Files under ./test/assets/ are no longer git-ignored.

Database backends

Added a read-only dataFolder property to database backends. Useful e.g. in tests.
Fixed an error when calling initStorage() more than once on the sqlite backend.

Other

Refactored chel eventsAfter and chel migrate using the new getBackend() helper (related to Make more use of getBackend to DRY things #20)
Added a minimal .vscode/settings.json file to make sure the Deno extension is enabled in VS Code / Codium.
Fixed missing --no-remote flag in deno task chel.
Made the isDir() and isFile() utilities async.

src/utils.ts

taoeffect · 2023-08-07T22:45:37Z

src/database-fs.ts

@@ -4,7 +4,7 @@ import { path } from './deps.ts'
 import { checkKey } from './utils.ts'

 // Initialized in `initStorage()`.
-let dataFolder = ''
+export let dataFolder = ''


Hmm, why is this being exported? 🤔

I don't see it imported anywhere... and isn't kind of a weird thing to do, to export such a variable?

I don't see it imported anywhere...

It's available as backend.dataFolder once the whole module has been imported as backend. It's not meant to be imported separately.

why is this being exported?

We might not need this in common use cases, but in some getBackend tests where the same backend object is "recycled" to use a new data folder, I could not easily make sure it had been correctly updated without having access to this propery.

Of course this happens because in this design, the backend object is the result of exporting its whole module, like export * as fs from './src/database-fs.ts'. In other words, the module acts as a singleton instance in OOP terminology, in which case exporting a module variable is like making a property of that singleton public rather than keeping it private.
So I guess it isn't that weird in the context of that design. But maybe that design itself is a bit weird.

By the way, the ES module spec ensures an exported let binding is read-only outside its module (although mutable from within it)

taoeffect

Very nicely done @snowteamer! Left a few change requests + questions

taoeffect · 2023-08-08T15:37:54Z

src/eventsAfter.ts

-  let from
-  let options
-  if (isDir(src)) {
-    from = 'fs'
-    options = { internal: true, dirname: src }
-  } else if (isFile(src)) {
-    from = 'sqlite'
-    options = { internal: true, dirname: path.dirname(src), filename: path.basename(src) }
-  } else throw new Error(`invalid argument: "${src}"`)
-
-  backend = backends[from as keyof typeof backends]
-  try {
-    await backend.initStorage(options)
-  } catch (error) {
-    exit(`could not init storage backend at "${src}" to fetch events from: ${error.message}`)
-  }


Always great to see massive chunks of code get deleted 😄

taoeffect · 2023-08-08T15:38:53Z

src/get.ts

+    const data = isURL(src)
+      ? await readRemoteData(src, key)
+      : await (await getBackend(src)).readData(key)


Reducing lines of code is always appreciated 👍

taoeffect · 2023-08-08T15:39:54Z

src/hash.ts

  if (!internal) {
-    console.log(`blake32Hash(${filename}):`, hash)
+    console.log(`CID for file (${filename}):`, cid)


Could you update this to read:

Suggested change

console.log(`CID for file (${filename}):`, cid)

console.log(`CID(${filename}):`, cid)

This makes it easier to use the command with programs like awk and maintains consistency with the format we had before.

taoeffect · 2023-08-08T15:41:58Z

src/migrate.ts

-
-  const backendFrom = backends[from as keyof typeof backends]
-  const backendTo = backends[to as keyof typeof backends]
-
-  if (!backendFrom) exit(`unknown storage backend: "${from}"`)
-  if (!backendTo) exit(`unknown storage backend: "${to}"`)
  if (from === to) exit('arguments --from and --to must be different')

-  if (isDir(src)) {
-    if (from === 'sqlite') exit(`not a database file: "${src}"`)
-  } else if (isFile(src)) {
-    if (from === 'fs') exit(`not a directory: "${src}"`)
-  } else {
-    exit(`not found: "${src}"`)
-  }
-
-  if (isDir(out)) {
-    if (to === 'sqlite') exit(`argument --out is a directory: "${out}"`)
-  } else if (isFile(out)) {
-    if (to === 'fs') exit(`argument --out is a file: "${out}"`)
-  } else if (out.endsWith('./')) {
-    if (to === 'sqlite') exit(`argument --out ends with a slash: "${out}"`)
-  }
-
-  try {
-    await backendFrom.initStorage(from === 'fs' ? {dirname: src} : {dirname: path.dirname(src), filename: path.basename(src)})
-  } catch (error) {
-    exit(`could not init storage backend at "${src}" to migrate from: ${error.message}`)
-  }


The amount of a DRYing brings a tear to my eye 🥲

taoeffect · 2023-08-08T15:50:29Z

src/utils.ts

 import * as fs from './database-fs.ts'
 import * as sqlite from './database-sqlite.ts'

-const backends = { fs, sqlite}


Thanks for fixing the spacing on the brace here 👍

taoeffect · 2023-08-08T15:51:05Z

src/utils.ts

+// We can update these constants later if we want.
+const backends = { fs, sqlite }
+const multibase = base58btc
+const multicodes = { JSON: 0x0200, RAW: 0x55 }


Could you please add a comment on the line above this one linking to where these values come from?

taoeffect · 2023-08-08T16:38:12Z

src/utils.ts

 export function isArrayLength (arg: number): boolean {
  return Number.isInteger(arg) && arg >= 0 && arg <= 2 ** 32 - 1
 }

 // Checks whether a path points to a directory, following symlinks if any.
-export function isDir (path: string | URL): boolean {
+export async function isDir (path: string | URL): Promise<boolean> {


Why did you change these to be async?

And if there's a strong reason to, can you add that as a comment?

Nowadays async is generally the default way for any I/O operation in JavaScript, and the fact these functions were sync caught my eye. Agreed, not sure whether there is a real performance benefit in the CLI tool use case, at least as long as the functions aren't called in loops. But in principe it still allows the engine to not forcibly stay idle while waiting for the operation to complete. And we might start using these functions in loops sooner or later, in which case being async enables usage of e.g. Promise.all().
Also these function names have no -Sync suffix unlike native Nodejs and Deno sync IO functions

Alright 👍

taoeffect · 2023-08-08T16:41:09Z

vendor/binary-install/index.js

-// nodejs as a wrapper executable to run our binary, greatly speeding up performance
-// see: https://github.com/EverlastingBugstopper/binary-install/tree/main/packages/binary-install
-
-const { existsSync, mkdirSync } = require("fs")


Why did you delete this file?

I'm pretty sure this file must not be deleted or it will break the install

Good catch, seems like I incidentally deleted it when re-vendoring the dependencies at some point

taoeffect · 2023-08-09T00:04:41Z

test/hash.test.ts

+        await t.step('it should create a json CID for a json file', async () => {
+            const filepath = './test/assets/hello.json'
+            const actual = await hash([filepath], true)
+            const expected = 'zyop8PQypgycyyCEcQsEHoc9Z8S9bQt1CuHuRKnXrAXagVBGxQyvH'
+            assertEquals(actual, expected)
+
+            const cid = CID.parse(actual, base58btc.decoder)
+            assertEquals(cid.code, 0x0200)
+            assertEquals(cid.byteLength, 39)
+            assertEquals(cid.multihash.code, 45600)
+            assertEquals(cid.version, 1)
+        })


👏 Bravo on these tests btw!!

This is definitely very much needed for sanity checking!!

taoeffect

LGTM!

snowteamer added 2 commits August 6, 2023 09:42

Add .vscode/settings.json

33501cd

Make hash() return CIDs for JSON files

013f725

taoeffect reviewed Aug 7, 2023

View reviewed changes

src/utils.ts Outdated Show resolved Hide resolved

snowteamer added 2 commits August 7, 2023 02:38

No longer call checkKey() in createCID()

e35af22

Refactor using getBackend() and add tests for it

4aef606

snowteamer force-pushed the add-json-codec-info branch from aab6889 to 4aef606 Compare August 7, 2023 22:28

snowteamer self-assigned this Aug 7, 2023

snowteamer changed the title ~~Use CIDs rather than plain hashes~~ Use CIDs for content addressing rather than plain hashes Aug 7, 2023

taoeffect reviewed Aug 7, 2023

View reviewed changes

taoeffect requested changes Aug 8, 2023

View reviewed changes

Address review

6a66f36

taoeffect reviewed Aug 9, 2023

View reviewed changes

This was referenced Aug 9, 2023

Figure out hash situation okTurtles/group-income#1604

Closed

Update info on how hashes are generated okTurtles/shelterprotocol.net-old#5

Open

taoeffect approved these changes Aug 9, 2023

View reviewed changes

taoeffect merged commit b3d653a into okTurtles:master Aug 9, 2023
1 check passed

snowteamer deleted the add-json-codec-info branch August 9, 2023 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use CIDs for content addressing rather than plain hashes #21

Use CIDs for content addressing rather than plain hashes #21

snowteamer commented Aug 6, 2023 •

edited

Loading

taoeffect Aug 7, 2023

snowteamer Aug 8, 2023

taoeffect left a comment

taoeffect Aug 8, 2023

taoeffect Aug 8, 2023

taoeffect Aug 8, 2023

snowteamer Aug 8, 2023

taoeffect Aug 8, 2023

taoeffect Aug 8, 2023

taoeffect Aug 8, 2023

taoeffect Aug 8, 2023

snowteamer Aug 8, 2023

taoeffect Aug 8, 2023

taoeffect Aug 8, 2023

snowteamer Aug 8, 2023 •

edited

Loading

taoeffect Aug 9, 2023

taoeffect left a comment

	console.log(`CID for file (${filename}):`, cid)
	console.log(`CID(${filename}):`, cid)

Use CIDs for content addressing rather than plain hashes #21

Use CIDs for content addressing rather than plain hashes #21

Conversation

snowteamer commented Aug 6, 2023 • edited Loading

Summary of changes

Content addressing

Testing (related to #11)

Database backends

Other

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taoeffect 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

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

snowteamer Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taoeffect left a comment

Choose a reason for hiding this comment

snowteamer commented Aug 6, 2023 •

edited

Loading

snowteamer Aug 8, 2023 •

edited

Loading