Refactor

[g-a-v-i-n]

I refactored index.js into a more functional style. I did this originally to find a bug where identical keys were returned from fullWalletFromSeed() for different ships. This should make this library easier to read and understand. fullWalletFromSeed() is especially easier to read. All functions have been refactored and a few new helper functions have been added. Structurally, the wallet is almost nearly the same but I made two changes:

1) The ownership node simply uses childNodeFromSeed() directly with ship and revision set to null. This means ownership node will have a meta field, a ship field and a revision field, even though they are unused.

2) Several functions' args have changed to a config object which is destructured internally.

I also added a few helper functions:

• bufferFrom(): wraps Buffer.from()
• bufferConcat(): wraps Buffer.concat()
• reverse(): wraps array.reverse()
• defaultTo(any, default): sets a default value if any is undefined
• get(): basically lodash get() but applies default if object itself is undefined.
• isUndefined()
• isNumber()

[g-a-v-i-n]

The semicolons are only occasionally necessary when writing a certain style of js where you chain methods together like in D3 or other 'weird' syntax cases, but normally it is very much reader preference. Part of why JS is 'so much fun'. Ted once told me he prefers semis because it clarifies when an expression ends. On the other hand, I'm very used to reading JS w/o semis.

[cgyarvin]

We should definitely pick a common, well-known, modern JS convention... not the expert here though. But I thought most JS people fear the implicit semicolon...

Sent from my iPhone

On Aug 28, 2018, at 12:31 PM, Fang notifications@github.com wrote:

@Fang- commented on this pull request.

Are end-of-line semicolons not fashionable anymore? Nobody told me! (And honestly, it looks kind of dangerous, like a gun with the safety off. That's probably irrational fear though.)

This looks great! I continue to be weirded out by modern-looking Javascript, but I must admit this is better than what I wrote. Do you want to do constitution-js next? (;

There is one question about the seed salt generation. Also, does the output from this match the output from the old version, when asking for just a single ship?

In src/index.js:

+ +const hash = (...args) => {

• // map args into buffers and concat into one buffer
• const buffer = bufferConcat(args.map(a => bufferFrom(a)))
• // generate a SHA-512 hash from input buffer
• return crypto.subtle.digest({ name: 'SHA-512' }, buffer) +}

• +const argon2u = (entropy, ticketSize) => argon2({

• pass: entropy, // string or Uint8Array
• salt: 'urbitkeygen',
• type: 10, // argon2.ArgonType.Argon2u,
• hashLen: ticketSize,
• // distPath: 'node_modules/argon2-wasm/dist', Does this actually work if you leave this commented out? If it does, great, feel free to remove this line.

In src/index.js:

• type: 10, // argon2.ArgonType.Argon2u,
• hashLen: ticketSize,
• // distPath: 'node_modules/argon2-wasm/dist',
• parallelism: 4,
• mem: 512000,
• time: 1, +})

• +const childSeedFromSeed = async config => {

• const { seed, type, revision, ship, password } = config
• const salt = typeof ship === 'number'
• ? ${salt}-${ship}
• : ${type}-${revision} This... doesn't match the previous implementation, I don't think? On line 54, salt is still empty, correct? We want type-revision always, and then -ship if we have one. If I'm reading this correctly it just gives you either -ship or type-revision.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[g-a-v-i-n]

@c-johnson probably knows more about js semicolons than I do. I think you're assuming common, well-known, modern JS conventions exist.

[c-johnson]

I have somehow managed to go my entire career without having a dog in this fight. Let's see how quickly I can convert myself.

Clicking on the first Google result, I find this answer:

https://stackoverflow.com/questions/34950322/use-of-semicolons-in-es6

Under our current babel version, this does indeed fail. So for first-class javascript applications written at Tlon (Section I of the Frontend Styleguide) , semicolons are now required.

However, if you review the Styleguide document (linked here: https://docs.google.com/document/d/14-XKvbv3KVovbPIghgaEXVyQBVFh7V9nso-cotCRcmY/edit), there's a loophole that Gavin could exploit under the "Section II" provisions of writing Library code. Since we can't force all Libraries we use to abide by our code style, he could technically complete this task under the messier Library definitions, and deliver this without semicolons.

After discussing this with Mark and some others, we've determined a need to split Section II into "Tlon-approved" Library code and "Messy NPM garbage" Library code. In other words, we will require strict code style guidelines for certain Libraries in the near future. Given this future development, I recommend that Gavin bite the bullet and practice writing with semicolons, so he doesn't have to go back and change this when we finalize the Tlon-approved library standards.

[cgyarvin]

dog in this holy war

I’m reminded of a time early in my career when I wanted to express my dissatisfaction with improving an overly mature codebase. I found myself describing this futile process as “putting lipstick on a dead horse.”

Sent from my iPhone

On Aug 28, 2018, at 1:25 PM, Christopher Johnson notifications@github.com wrote:

I have somehow managed to go my entire career without having a dog in this holy war. Let's see how quickly I can convert myself.

Clicking on the first Google result, I find this answer:

https://stackoverflow.com/questions/34950322/use-of-semicolons-in-es6

Under our current babel version, this does indeed fail. So for first-class javascript applications written at Tlon (Section I of the Frontend Styleguide) , semicolons are now required.

However, if you review the Styleguide document (linked here: https://docs.google.com/document/d/14-XKvbv3KVovbPIghgaEXVyQBVFh7V9nso-cotCRcmY/edit), there's a loophole that Gavin could exploit under the "Section II" provisions of writing Library code. Since we can't force all Libraries we use to abide by our code style, he could technically complete this task under the messier Library definitions, and deliver this without semicolons.

After discussing this with Mark and some others, we've determined a need to split Section II into "Tlon-approved" Library code and "Messy NPM garbage" Library code. In other words, we will require strict code style guidelines for certain Libraries in the near future. Given this future development, I recommend that Gavin bite the bullet and practice writing with semicolons, so he doesn't have to go back and change this when we finalize the Tlon-approved library standards.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[g-a-v-i-n]

Ok, semis are in, I also commented all the functions.

[vvisigoth]

If only all holy wars were this civil. Merging.

[Fang-]

But he hadn't removed the // distPath yet! ):

That's fine though. I'm going to assume the answer to this question of mine was yes:

does the output from this match the output from the old version, when asking for just a single ship?

If not, there's likely an issue in the hoon version of this, too...