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

NNS1-2886: Remove nns-proto dependency from @dfinity/nns #599

Merged
merged 5 commits into from
Mar 28, 2024

Conversation

dskloetd
Copy link
Collaborator

Motivation

The NNS dapp now requires version 2.4.9 of the Internet Computer Ledger app.
See for context: https://forum.dfinity.org/t/nns-dapp-to-remove-protobuf-dependency-upgrade-your-ledger-ic-app-to-2-4-9/27712
This means that we no longer need to use a separate code path with protocol buffers when making hardware wallet transactions but can instead use the exact same code path as for normal transactions.

NNS-dapp already doesn't use hardware wallet specific code paths since dfinity/nns-dapp#4373

Changes

  1. Remove the dependency from @dfinity/nns on @dfinity/nns-proto.
  2. Remove the hardware wallet specific code paths. Hardware wallets will use the standard candid code.
  3. Remove code that becomes unused as a result.

Note that there is one slight behavior change. Previously listNeurons would throw FeatureNotSupportedError if you try to call it with certified == false with this.hardwareWallet == true. Since we no longer have this.hardwareWallet, we can't do this anymore. The hardware wallet will now return an empty signature and it's up to the signing identity to throw an error as a response. NNS dapp does this, but also doesn't call listNeurons with certified = false.

Tests

Hardware wallet specific tests have been removed.

Todos

  • Add entry to changelog (if necessary).

Copy link
Contributor

github-actions bot commented Mar 28, 2024

size-limit report 📦

Path Size
@dfinity/ckbtc 7.29 KB (0%)
@dfinity/cketh 2.17 KB (0%)
@dfinity/cmc 1.29 KB (0%)
@dfinity/ledger-icrc 3.4 KB (0%)
@dfinity/ledger-icp 14.77 KB (0%)
@dfinity/nns 33.64 KB (-5.68% 🔽)
@dfinity/nns-proto 76.44 KB (0%)
@dfinity/sns 15.43 KB (0%)
@dfinity/utils 4.44 KB (0%)
@dfinity/ic-management 2.61 KB (0%)

@dskloetd dskloetd marked this pull request as ready for review March 28, 2024 10:15
@dskloetd dskloetd requested a review from mstrasinskis March 28, 2024 10:15
Copy link
Contributor

@mstrasinskis mstrasinskis left a comment

Choose a reason for hiding this comment

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

Nice 👍

@dskloetd dskloetd merged commit be5ccfe into main Mar 28, 2024
11 checks passed
@dskloetd dskloetd deleted the kloet/nns-proto branch March 28, 2024 10:44
@peterpeterparker
Copy link
Member

Cool 👍

That was the last PR? In next version of ic-js we should mark nns-proto library as deprecated (here and on npm).

@dskloetd
Copy link
Collaborator Author

This should be the last PR on ic-js. I'm going to do some manual testing on NNS dapp to make sure everything still work with the HW and then update nns-dapp.

@peterpeterparker
Copy link
Member

Super!

@dskloetd
Copy link
Collaborator Author

Manually tested that the following still work with hardware wallet:

  • transfer
  • listNeurons
  • increaseDissolveDelay
  • startDissolving
  • stopDissolving
  • disburse
  • spawnNeuron
  • addHotkey
  • removeHotkey

Do you want to deprecate the package? (or point me how to do it)

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