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

Update old_list_neurons_service.certified.idl.js #729

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

dskloetd
Copy link
Collaborator

@dskloetd dskloetd commented Oct 11, 2024

Motivation

The purpose of old_list_neurons_service.certified.idl.js is to have the request type be supported by Ledger HW ICP app version 2.4.9.
The hardware wallet does care about the response type so it is safe to update.
We want to include visibility specifically.

Changes

Recreate old_list_neurons_service.certified.idl.js the same way it was created the first time as documented by the comment at the top:

// This file was created manually by taking governance.certified.idl.js and
// removing everything that isn't needed for `list_neurons` and then removing
// all fields except `neuron_ids` and `include_neurons_readable_by_caller` from
// `ListNeurons`.
// The Ledger hardware wallet app verion 2.4.9 doesn't support the newer fields,
// even when they are optional and not set, so we use this service for
// compatibility with the hardware wallet.

The result is that the visibility field was added in addition to field names now being wrapped in ''.

Tests

Used manually with nns-dapp and was able to

  1. show neurons from a hardware wallet,
  2. see visibility in the response.

Todos

  • Add entry to changelog (if necessary).
    not necessary

Copy link
Contributor

size-limit report 📦

Path Size
@dfinity/ckbtc 7.99 KB (0%)
@dfinity/cketh 3.53 KB (0%)
@dfinity/cmc 1.3 KB (0%)
@dfinity/ledger-icrc 4.17 KB (0%)
@dfinity/ledger-icp 15.43 KB (0%)
@dfinity/nns 36.23 KB (+0.02% 🔺)
@dfinity/nns-proto 140.98 KB (0%)
@dfinity/sns 15.87 KB (0%)
@dfinity/utils 4.5 KB (0%)
@dfinity/ic-management 3.01 KB (0%)

@dskloetd dskloetd marked this pull request as ready for review October 11, 2024 12:27
@dskloetd dskloetd requested review from a team as code owners October 11, 2024 12:27
@dskloetd dskloetd enabled auto-merge (squash) October 11, 2024 13:50
@dskloetd dskloetd merged commit 74e0223 into main Oct 11, 2024
11 checks passed
@dskloetd dskloetd deleted the kloet/old-visibility branch October 11, 2024 13:55
dskloetd added a commit that referenced this pull request Oct 11, 2024
# Motivation

I updated the comment to be more accurate when updating the service file
in #729 but then forgot to commit
these changes.

# Changes

Improve the comment's accuracy.

# Tests

N/A

# Todos

- [ ] Add entry to changelog (if necessary).
not necessary
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.

2 participants