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

Pin exact dependency versions #1217

Merged
merged 5 commits into from
Dec 18, 2023

Conversation

gtsonevv
Copy link
Collaborator

@gtsonevv gtsonevv commented Dec 15, 2023

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

Test Plan

Related issues/PRs

#1200

@gtsonevv gtsonevv requested review from frol and gagdiez December 15, 2023 11:42
Copy link

changeset-bot bot commented Dec 15, 2023

⚠️ No Changeset found

Latest commit: 90bec46

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

"buffer": "^6.0.3",
"elliptic": "^6.5.4",
"buffer": "6.0.3",
"elliptic": "6.5.4",

Choose a reason for hiding this comment

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

why is elliptic used here in ed25519 context? elliptic's implementation of the curve is buggy and produces invalid outputs, which means in blockchain context "someone will lose money"

@frol
Copy link
Collaborator

frol commented Dec 15, 2023

From the library usage perspective, it is not great to pin all the dependencies down to a single version as that may block deduplicating the dependencies if two libraries of slightly different versions are used (1.0.1 and 1.0.2). I am wondering if there is some middle ground that we can find here.

Also, this PR only resolves the first part of #1200, so it will be too early to close that issue without reviewing and removing buggy dependencies.

@paulmillr
Copy link

paulmillr commented Dec 15, 2023

deduplicating the dependencies

Just walk through every dependency in the lockfile and see which are duplicated, then resolve every one separately - that's what I do.

https://npmgraph.js.org/?q=near-api-js

@gtsonevv
Copy link
Collaborator Author

Hey guys, thank you for the replies. Based on them these are the next steps that we can take:

  1. Our team will go through the dependency graph and resolve duplications.
  2. We will try to reduce the bundle size. Looking at https://bundlephobia.com/package/[email protected] we can see that 10% is bn.js which should be replaceable by native JS BigNumber. 7% is tweetnacl which is already replaced. We will try to find a comparable replacement for ajv because it’s biggest library by far(30%).

@paulmillr
Copy link

others:

  • js-sha256 => noble-hashes (already used)
  • elliptic => noble-curves (already used)
  • borsch deps
    • remove bn.js (native BigInt)
    • remove text-encoding-utf-8 (native TextEncoder)
    • bs58 should be upgraded upstream to remove safe-buffer, or replaced with scure-base
  • re-evaluate need for node-fetch
  • re-evaluate need for error-polyfill

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

As suggested offline, let's merge this PR with the pinned dependencies, and then work on a separate PR that will slim down the dependencies.

@gtsonevv gtsonevv merged commit a8f4a29 into near:master Dec 18, 2023
1 check passed
@gtsonevv gtsonevv deleted the use-lock-down-dependency-versions branch December 18, 2023 14:59
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.

3 participants