Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

feat: type with libp2p-interfaces #191

Closed
wants to merge 1 commit into from
Closed

feat: type with libp2p-interfaces #191

wants to merge 1 commit into from

Conversation

Gozala
Copy link

@Gozala Gozala commented Jun 29, 2021

This is fallout of the libp2p/js-libp2p#955 effort. More specifically it types everything via crypto interfaces definitions from libp2p/js-libp2p-interfaces#100 which is necode to eded to align it with a pull that integrates interfaces in js-libp2p

Reviewer notes

  • This removes hand written typedefs and generates those instead with aegir (so everything is typed now)
  • Bunch of places did something like new Uint8Array(buffer, buffer.byteOffset, buffer.byteLength) where buffer was instance of ArrayBufferwhich does not have byteOffset property. It worked correctly because undefined was triggering same code path as new Uint8Array(buffer)
  • Private/Public keys were added algorithm property (introduced by interface) which follows the same pattern as base encoders, hashers and block codecs (in multiformats). This allows narrow down type without resorting to instanceof, which is not a thing when embracing interfaces as opposed to implementations.
  • Some functions took e.g. CurveType | string style args, now they are strict and just take CurveType which enables type checker to catch problems at compile time. Users generally can cast string to specific literals when needed.
  • src/keys/keys.d.ts is generated now via pbjs
  • Bunch of ts-ignores are used when importing node-forge sub-modules because there are not typedefs for those. I assume reason why we don't simply require('node-forge') is so that we can avoid pulling unnecessary code, if that is not the case maybe we should switch to using it instead.
  • Had to hoist Secp256k1PublicKey / Secp256k1PrivateKey because TS doesn't support non static (generate) classes as their type can't really be expressed. I would personally not even expose classes which would also address the issue, but that seemed to invasive change.
    • Do we even need the whole factory thing there, seems like it unnecessarily complication.
  • generateKeyPairFromSeed no longer takes 3rd parameter because nothing was using it.
  • RSA keys had strange genSecret method. It made little sense to me so I've omitted it from the interface (no other key implements it) and created a static function on the module which is what test is using now. I kept the method around just in case, but type checker will complain if you try to use it.
  • I've marked some functions (that made little sense to me) deprecated, they will just show-up crossed out in the vscode. See more in suggestions

Suggestions

  • I feel like src/keys/index.js attempts to provide a generic API where it makes no sense. E.g. generateKeyPairFromSeed can only be used by Ed25519 keys, so it throws in all other cases. generateKeyPair ignores bits when secp256k1 is used.

    All this is just suggests that those APIs aren't generic, it would make a lot more sense to just use ed25519.generateKeyPairFromSeed and not have generic equivalent. I think same is true for generateKeyPair there is questionable value in indirection here:

    keys.generateKeyPairFromSeed('Ed25519', seed)
    // VS
    ed25519.generateKeyPairFromSeed(seed)

    Which is also true for generateKeyPair.

  • I think export function on private keys is another source of indirection that complicates things instead of simplifying them more on this in Proposal: Separate key export functionality #190

@@ -34,6 +43,7 @@ module.exports = async (cipherType, hash, secret) => {
throw errcode(new Error('missing hash type'), 'ERR_MISSING_HASH_TYPE')
}

// @ts-expect-error - Blowfish has no keySize
Copy link
Author

Choose a reason for hiding this comment

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

@vasco-santos this seems like a bug that needs addressing

const buffer = new forge.util.ByteBuffer(this.marshal())
const asn1 = forge.asn1.fromDer(buffer)
const privateKey = forge.pki.privateKeyFromAsn1(asn1)

const options = {
return forge.pki.encryptRsaPrivateKey(privateKey, password, {
Copy link
Author

Choose a reason for hiding this comment

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

Note: This allows TS to infer literals instead of treating them as strings.

@BigLep
Copy link

BigLep commented Aug 27, 2021

Next steps:

  1. @Gozala to address merge conflicts
  2. @achingbrain to review

@lidel lidel marked this pull request as draft September 10, 2021 14:11
@BigLep
Copy link

BigLep commented Nov 12, 2021

2021-11-12 update: we do still want this work, but it needs to be looked at in a couple weeks as part of larger js-libp2p typing changes that will be occurring.

@lidel
Copy link
Member

lidel commented Jan 21, 2022

JS triage note: closing, no longer relevant as we converted to TypeScript recently

@lidel lidel closed this Jan 21, 2022
@lidel lidel deleted the feat/interface branch January 21, 2022 15:58
@BigLep
Copy link

BigLep commented Jan 21, 2022

For reference, the push to TS in js-libp2p is happening in libp2p/js-libp2p#1021

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants