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

Add Secp256k1 support #1355

Merged
merged 30 commits into from
Jul 22, 2024
Merged

Conversation

gtsonevv
Copy link
Collaborator

@gtsonevv gtsonevv commented Jun 26, 2024

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • Commit messages follow the conventional commits spec
  • If this is a code change: I have written unit tests.
  • If this changes code in a published package: I have run pnpm changeset to create a changeset JSON document appropriate for this change.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Adds support for secp256k1 curve

Test Plan

All tests pass

Related issues/PRs

#985
#1039
#440

@gtsonevv gtsonevv requested a review from vikinatora June 26, 2024 06:16
Copy link

changeset-bot bot commented Jun 26, 2024

🦋 Changeset detected

Latest commit: ac393c7

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

This PR includes changesets to release 12 packages
Name Type
@near-js/crypto Minor
@near-js/accounts Patch
@near-js/biometric-ed25519 Patch
@near-js/keystores Patch
near-api-js Patch
@near-js/signers Patch
@near-js/transactions Patch
@near-js/wallet-account Patch
@near-js/keystores-browser Patch
@near-js/keystores-node Patch
@near-js/cookbook Patch
@near-js/providers 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

@gagdiez
Copy link
Contributor

gagdiez commented Jun 26, 2024

I gave it a quick look, do we have any test against the chain using secp256k1? I think the current tests only look for internal consistency (i.e. what we signed, can be checked) but not necessarily for external consistency (what we signed, the chain can understand)

Making a small transfer after creating the account should be sufficient to prove correctness

vikinatora
vikinatora previously approved these changes Jul 11, 2024
Copy link
Collaborator

@andy-haynes andy-haynes left a comment

Choose a reason for hiding this comment

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

Nice work!

I do think we should reexamine the original approach to avoid introducing unnecessary breaking changes and complexity in serialization, but I'm keen to hear other thoughts. If this does end up being the most desirable approach then at a minimum we should avoid breaking backward compatibility with missing class fields.

packages/biometric-ed25519/src/index.ts Outdated Show resolved Hide resolved
packages/crypto/package.json Show resolved Hide resolved
packages/crypto/src/public_key.ts Outdated Show resolved Hide resolved
packages/keystores/src/in_memory_key_store.ts Outdated Show resolved Hide resolved
packages/transactions/src/schema.ts Show resolved Hide resolved
Copy link
Collaborator

@andy-haynes andy-haynes left a comment

Choose a reason for hiding this comment

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

Thank you for the clarifications on the implementation. Just a couple more things otherwise I think we're good to go 🚀

packages/crypto/src/public_key.ts Outdated Show resolved Hide resolved
packages/crypto/src/public_key.ts Show resolved Hide resolved
packages/crypto/src/public_key.ts Outdated Show resolved Hide resolved
packages/crypto/src/public_key.ts Outdated Show resolved Hide resolved
packages/transactions/src/signature.ts Show resolved Hide resolved
@andy-haynes
Copy link
Collaborator

@race-of-sloths 🦥

@race-of-sloths
Copy link

race-of-sloths commented Jul 12, 2024

@gtsonevv Thank you for your contribution! Your pull request is now a part of the Race of Sloths!
Are you going to win race-of-sloths this month? If so, you should speed up! Run, Sloth, run!

Shows profile picture for the author of the PR

Current status: executed
Reviewer Score
@andy-haynes 5

The average score is 5

@gtsonevv check out your results on the Race of Sloths Leaderboard! and in the profile

What is the Race of Sloths

Race of Sloths is a friendly competition where you can participate in challenges and compete with other open-source contributors within your normal workflow

For contributors:

  • Tag @race-of-sloths inside your pull requests
  • Wait for the maintainer to review and score your pull request
  • Check out your position in the Leaderboard
  • Keep weekly and monthly streaks to reach higher positions
  • Boast your contributions with a dynamic picture of your Profile

For maintainers:

  • Score pull requests that participate in the Race of Sloths
  • Engage contributors with fair scoring and fast responses so they keep their streaks
  • Promote the Race to the point where the Race starts promoting you
  • Grow the community of your contributors

Feel free to check our website for additional details!

Bot commands
  • For contributors
    • Include a PR: @race-of-sloths include to enter the Race with your PR
  • For maintainers:
    • Assign points: @race-of-sloths score [1/2/3/5/8/13] to award points based on your assessment.
    • Reject this PR: @race-of-sloths exclude to send this PR back to the drawing board.
    • Exclude repo: @race-of-sloths pause to stop bot activity in this repo until @race-of-sloths unpause command is called

@andy-haynes
Copy link
Collaborator

@race-of-sloths score 5

@race-of-sloths
Copy link

🌟 Score recorded!

@andy-haynes, thank you for scoring this pull request in the Race of Sloths!

andy-haynes
andy-haynes previously approved these changes Jul 17, 2024
Copy link
Collaborator

@andy-haynes andy-haynes left a comment

Choose a reason for hiding this comment

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

Awesome work! Just noticed two duplicated tests otherwise everything's good to go

const keyString = 'secp256k1:7s1Jno8tbqFHBMqLh3epaFBbk194zAuMqo8yPbxvTbXn';
const keyPair2 = KeyPair.fromString(keyString);
expect(keyPair2.toString()).toEqual(keyString);
});
});

test('test from secret', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These can be removed now that they're in the describe block above

@andy-haynes
Copy link
Collaborator

@gtsonevv would you mind rebasing this please? It should just be your work + release ahead of you in history.

@gtsonevv
Copy link
Collaborator Author

@andy-haynes rebase onto master?

@andy-haynes andy-haynes merged commit cc93136 into near:master Jul 22, 2024
1 check passed
@race-of-sloths
Copy link

✅ PR is finalized!

Your contribution is much appreciated with a final score of 5!
You have received 50 Sloth points for this contribution

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

Successfully merging this pull request may close these issues.

5 participants