Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Secp256k1 support PR rebased #985

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

esaminu
Copy link
Collaborator

@esaminu esaminu commented Sep 13, 2022

Motivation

Rebased version of #810

Description

Adds support for secp256k1 curve

Checklist

  • Read the contributing guidelines
  • Commit messages follow the conventional commits spec
  • Performed a self-review of the PR
  • Added automated tests
  • Manually tested the change

@changeset-bot
Copy link

changeset-bot bot commented Sep 21, 2022

🦋 Changeset detected

Latest commit: 018fe99

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
near-api-js Minor
@near-js/cookbook Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

yarn.lock Outdated Show resolved Hide resolved
@@ -68,7 +69,7 @@
"files": [
{
"path": "dist/near-api-js.min.js",
"maxSize": "105kB"
"maxSize": "250kB"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like a very big jump, is this really necessary? Do you know what size we're at currently?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We went from 68.23KB in current master to 174.92KB which is a big jump due to the secp256k1 dependency https://bundlephobia.com/package/secp256k1@4.0.3

@MaximusHaximus thoughts on how if at all this affects how we release this?

packages/near-api-js/src/utils/key_pair.ts Outdated Show resolved Hide resolved
packages/near-api-js/src/utils/key_pair.ts Outdated Show resolved Hide resolved
packages/near-api-js/src/transaction.ts Show resolved Hide resolved
packages/near-api-js/src/utils/key_pair.ts Outdated Show resolved Hide resolved
@morgsmccauley
Copy link
Collaborator

We'll also need a .changeset file 😄

@morgsmccauley
Copy link
Collaborator

Sorry misclick

"near-api-js": minor
---

Add support for the secp256k1 curve
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be good to add more detail here, can we please expand on this? Maybe something along the lines of: "Add support for generating secp256k1 key pairs for signing and verification of messages". Not super familiar with the changes so maybe there is more.

@waynenilsen
Copy link

we will need this also

@gtsonevv
Copy link
Collaborator

Hey @frol, if this PR is ready to merge, we can create a new one and move the changes there again.

@frol
Copy link
Collaborator

frol commented Feb 10, 2024

@gtsonevv Oh, I missed your message, sorry for the delay. I believe you are in a better position to judge if it is ready to merge. To be honest, I have never used secp256k1 keys on NEAR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog 🥶
Development

Successfully merging this pull request may close these issues.

None yet

7 participants