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

feat: use noble-secp256k1 and noble-ed25519 #202

Merged
merged 12 commits into from
Oct 21, 2021
Merged

Conversation

hugomrdias
Copy link
Member

@hugomrdias hugomrdias commented Oct 13, 2021

@vasco-santos
Copy link
Member

this is rad! 🎉 it also seems faster

@achingbrain
Copy link
Member

Looks great, just need to fix up CI.

@hugomrdias
Copy link
Member Author

can i just change the tests to the new error messages ?

@achingbrain
Copy link
Member

achingbrain commented Oct 13, 2021

Sounds good, in the absence of something that will make these tests less fragile - error codes, etc.

@dignifiedquire
Copy link
Member

Had a brief look at the code of the library, and it looks quite solid, including an audit from cure53. So I am 👍 on this for what it's worth.

@hugomrdias hugomrdias marked this pull request as ready for review October 14, 2021 10:22
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

just needs a lint tweak

@hugomrdias
Copy link
Member Author

humm what do we do with the ursa stuff ?

@hugomrdias hugomrdias changed the title feat: use noble-secp256k1 feat: use noble-secp256k1 and noble-ed25519 Oct 14, 2021
@achingbrain
Copy link
Member

#203 should unblock the build

@achingbrain
Copy link
Member

Ha, maybe not. Well, these are actual test failures rather than build ones so that's some progress, I guess.

@hugomrdias
Copy link
Member Author

i will check it asap

@hugomrdias
Copy link
Member Author

@achingbrain @vasco-santos should be good to go

src/keys/rsa-class.js Outdated Show resolved Hide resolved
src/keys/ed25519.js Outdated Show resolved Hide resolved
await secp256k1Crypto.hashAndVerify(privKey, sig, null)
} catch (err) {
return // expected
}
throw new Error('Expected error to be thrown')
})

it('errors when validating a message with an invalid signature', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

why removing this?

Copy link
Member Author

Choose a reason for hiding this comment

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

i just moved the test up a bit check 'does not validate when validating a message with an invalid signature', because it doesnt throw now just returns false for an invalid sig

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, so this now returns a boolean false instead of throwing an error. Can we flag that breaking change in the commit message?

Copy link
Member Author

Choose a reason for hiding this comment

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

can you do it when you squash and merge ?

Copy link
Member

Choose a reason for hiding this comment

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

sure, I can do

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM

@vasco-santos vasco-santos merged commit 167eace into master Oct 21, 2021
@vasco-santos vasco-santos deleted the feat/noble-secp branch October 21, 2021 14:58
achingbrain added a commit to ipfs-examples/js-ipfs-examples that referenced this pull request Dec 3, 2021
The `babel-plugin-transform-exponentiation-operator ` Babel plugin transforms `1n ** 1n` into `Math.pow(1n, 1n)` which doesn't work because `Math.pow` doesn't support BigInts - facebook/create-react-app#6907 (comment)

This is used in `@noble/ed25519` which recently got added to `libp2p-crypto` - libp2p/js-libp2p-crypto#202

The fix is to change the `"browsersList"` setting to exclude ie and old android - hirosystems/stacks.js#1096 (comment)
github-actions bot pushed a commit to ipfs-examples/js-ipfs-browser-create-react-app that referenced this pull request Dec 3, 2021
The `babel-plugin-transform-exponentiation-operator ` Babel plugin transforms `1n ** 1n` into `Math.pow(1n, 1n)` which doesn't work because `Math.pow` doesn't support BigInts - facebook/create-react-app#6907 (comment)

This is used in `@noble/ed25519` which recently got added to `libp2p-crypto` - libp2p/js-libp2p-crypto#202

The fix is to change the `"browsersList"` setting to exclude ie and old android - hirosystems/stacks.js#1096 (comment)
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.

4 participants