urbit / urbit-key-generation

Key derivation and HD wallet generation functions for Urbit
MIT License
15 stars 8 forks source link

Build doesn't seem to be on npm #80

Closed g-a-v-i-n closed 4 years ago

g-a-v-i-n commented 4 years ago

npm install urbit-key-generation in Bridge doesn't 'install' the /dist folder, which makes me wonder if npm run build was ran before publishing to npm.

Any thoughts @jtobin?

g-a-v-i-n commented 4 years ago

On closer inspection, this seems to be a compound issue. 1) there is no /dist on npm 2) webpack cannot use the browser field in package.json at this time. It uses main to locate the build, and main points to the source.

Bridge compiles successfully when the above are resolved.

g-a-v-i-n commented 4 years ago

I'm also getting issues with urbit-ob, despite it being up-to-date here with urbit-ob@4.1.1. Some things are under the co export and others are under ob.

Edit: on closer inspection, all exports are included under ob.

vvisigoth commented 4 years ago

Interesting. Wonder why this didn't show up in development?

On Wed, Sep 4, 2019 at 6:35 PM g-a-v-i-n notifications@github.com wrote:

I'm also getting issues with urbit-ob, despite it being up-to-date here with urbit-ob@4.1.1

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/urbit/urbit-key-generation/issues/80?email_source=notifications&email_token=AAMJBY45G5VFBCCXMKJ4UIDQIBO4PA5CNFSM4ITYAEA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD55QY6A#issuecomment-528157816, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMJBY7E5FFHOCDFMW275EDQIBO4PANCNFSM4ITYAEAQ .

-- — ~poldec-tonteg http://urbit.org

vvisigoth commented 4 years ago

PaperRenderer development, that is.

On Wed, Sep 4, 2019 at 6:39 PM Anthony Arroyo anthony@tlon.io wrote:

Interesting. Wonder why this didn't show up in development?

On Wed, Sep 4, 2019 at 6:35 PM g-a-v-i-n notifications@github.com wrote:

I'm also getting issues with urbit-ob, despite it being up-to-date here with urbit-ob@4.1.1

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/urbit/urbit-key-generation/issues/80?email_source=notifications&email_token=AAMJBY45G5VFBCCXMKJ4UIDQIBO4PA5CNFSM4ITYAEA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD55QY6A#issuecomment-528157816, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMJBY7E5FFHOCDFMW275EDQIBO4PANCNFSM4ITYAEAQ .

-- — ~poldec-tonteg http://urbit.org

-- — ~poldec-tonteg http://urbit.org

g-a-v-i-n commented 4 years ago

@vvisigoth I believe this is because we only run urbit-key-generation on a node server in PaperRenderer development. Although I still can't explain why the tests fail, other than bundling issues.

This brings up a few questions:

  1. Should ob-js and urbit-key-generation be bundled in a standardized way?
  2. Having co and ob in the same lib is weird for ontological reasons. Should co + ob be exported as ob?
  3. Should tests point to /src or to /dist?
g-a-v-i-n commented 4 years ago

When I run tests in urbit-key-generation, I get the following failures.

  4) deriveNodeKeys
       derives by paths correctly:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/gavinatkinson/tlon/keygen-js/test/test.js)

  5) generateOwnershipWallet
       generates wallets as expected:
     TypeError: ob.patq2hex is not a function
      at Object.generateOwnershipWallet (dist/index.js:3576:567)
      at Context.<anonymous> (test/test.js:443:27)

  6) generateWallet
       generates wallets as expected:
     TypeError: ob.patq2hex is not a function
      at shard (dist/index.js:3554:119)
      at Object.generateWallet (dist/index.js:3590:814)
      at Context.<anonymous> (test/test.js:489:27)
jtobin commented 4 years ago

npm install urbit-key-generation in Bridge doesn't 'install' the /dist folder, which makes me wonder if npm run build was ran before publishing to npm.

Huh, very strange. I have a prepublishOnly script in package.json that creates the browser build before publishing to npm -- that has always worked fine, so I'm not sure why we're missing the browser build for this version. I'll see what's up with that.

jtobin commented 4 years ago

webpack cannot use the browser field in package.json at this time. It uses main to locate the build, and main points to the source.

IIRC this is true, but the browser field is useful for rollup. Webpack should work fine using the setup that's there (it has done so for a number of versions, after all).

jtobin commented 4 years ago

When I run tests in urbit-key-generation, I get the following failures.

I can't reproduce this -- I wiped my package-lock.json and node_modules just to confirm, but the tests pass every time.

Do you possibly have urbit-ob 4.1.0 floating around? That's the version where ob and co got screwed up on export. We should use stricter version pinning here anyway (e.g. 4.1.1 instead of ^4.1.1).

jtobin commented 4 years ago

Should ob-js and urbit-key-generation be bundled in a standardized way?

I think urbit-ob is fine. urbit-key-generation is also "fine", in that it typically works great with zero effort, but there are probably some improvements that could be made to the build (see #39, #76, #40).

(That said, it's not necessarily that easy. urbit-key-generation has some dependencies that themselves have circular dependencies, so building with rollup was a nightmare when I last tried it.)

jtobin commented 4 years ago

having co and ob in the same lib is weird for ontological reasons. Should co + ob be exported as ob?

The funny thing is that the co functions are what people are actually interested in here. ob just implements the raw generalised Feistel cipher that everyone knows and loves from the @pocalypse, but which doesn't really get used outside the @p-swizzling case.

jtobin commented 4 years ago

Ah. npm, for reasons I can't even begin to fathom, ignores files in .gitignore.

So 66db0b38034a26d405c6235690608c6c316185bb, which added the dist directory to .gitignore, apparently caused this one.

The solution here is to apparently create an empty .npmignore file, which npm will prefer over .gitignore. Good grief.

ohAitch commented 4 years ago

I think one of the standard uses of .gitignore is putting passwords in a secrets.env file, so can't fault them too much there!

On Thu, Sep 5, 2019 at 03:47, Jared Tobin notifications@github.com wrote:

Closed #80 https://github.com/urbit/urbit-key-generation/issues/80 via

82 https://github.com/urbit/urbit-key-generation/pull/82.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/urbit/urbit-key-generation/issues/80?email_source=notifications&email_token=AAOFPBUKLG2FGCSOSCXMB33QIDPVZA5CNFSM4ITYAEA2YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOTOJ2LUY#event-2610144723, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOFPBQJX43NGWPHWLYT3XLQIDPVZANCNFSM4ITYAEAQ .

jtobin commented 4 years ago

In the words of (the elder) Trudeau, just watch me.

continues faulting

😄

ohAitch commented 4 years ago

May the fault be not in ourselves, but in our stars, zod willing

On Thu, Sep 5, 2019 at 09:55, Jared Tobin notifications@github.com wrote:

In the words of (the elder) Trudeau, just watch me https://en.wikipedia.org/wiki/Just_watch_me.

continues faulting

😄

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/urbit/urbit-key-generation/issues/80?email_source=notifications&email_token=AAOFPBSHWKV3FBIIWMGAI7LQIE2ZLA5CNFSM4ITYAEA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD572UKI#issuecomment-528460329, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOFPBV247XW6CRYJPCRACTQIE2ZLANCNFSM4ITYAEAQ .

g-a-v-i-n commented 4 years ago

Wow that .gitignore issue is wild. Thanks for resolving this issue, but I'm still having problems with the use of the browser field. I made a PR to resolve this, starting with ob-js.