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

icrc index getLedgerId #693

Merged
merged 7 commits into from
Aug 10, 2024
Merged

icrc index getLedgerId #693

merged 7 commits into from
Aug 10, 2024

Conversation

mstrasinskis
Copy link
Contributor

@mstrasinskis mstrasinskis commented Aug 9, 2024

Motivation

For the import token functionality, we need to ensure that the provided by users index canister is correctly associated with the same token and not mistakenly linked to a different ledger canister. To achieve this, we going to use the ledger_id API of the index canister, which returns the corresponding ledger canister ID. This PR introduces a function that makes the ledger_id request.

Changes

  • Add getLedgerId function.

Tests

  • Test for getLedgerId.
  • Tested manually in nns-dapp against localhost.

Todos

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

@mstrasinskis mstrasinskis requested review from a team as code owners August 9, 2024 15:51
@mstrasinskis mstrasinskis changed the title Mstr/ledger icrc index getLedgerId Aug 9, 2024
@peterpeterparker
Copy link
Member

We already do something like that in Oisy. Let me check.

Copy link
Contributor

github-actions bot commented Aug 9, 2024

size-limit report 📦

Path Size
@dfinity/ckbtc 7.91 KB (0%)
@dfinity/cketh 3.45 KB (0%)
@dfinity/cmc 1.29 KB (0%)
@dfinity/ledger-icrc 3.89 KB (-0.13% 🔽)
@dfinity/ledger-icp 15.23 KB (0%)
@dfinity/nns 36.32 KB (0%)
@dfinity/nns-proto 140.98 KB (0%)
@dfinity/sns 15.9 KB (0%)
@dfinity/utils 4.47 KB (0%)
@dfinity/ic-management 2.87 KB (0%)

@peterpeterparker
Copy link
Member

Indeed @mstrasinskis, a function ledgerId is already exposed by @dfinity/ledger-icrc.

See here https://github.com/dfinity/ic-js/tree/main/packages/ledger-icrc#gear-ledgerid

…edger-id

# Conflicts:
#	packages/ledger-icrc/src/index.canister.ts
@dskloetd
Copy link
Collaborator

dskloetd commented Aug 9, 2024

Looks like this PR adds it on the non-ng index canister which does not seem to have it yet.

@mstrasinskis
Copy link
Contributor Author

@peterpeterparker, thank you for pointing this out. However, as @dskloetd already mentioned, this PR exposes the API on the non-NG canister, which is used in nns-dapp.

@peterpeterparker
Copy link
Member

Sounds good. Can you use the same naming ledgerId for consistency reasons?

@mstrasinskis
Copy link
Contributor Author

Sounds good. Can you use the same naming ledgerId for consistency reasons?

Good idea, this way it will be easier to switch to the NG canister later.

Copy link
Member

@peterpeterparker peterpeterparker left a comment

Choose a reason for hiding this comment

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

LGTM, thx

@mstrasinskis mstrasinskis merged commit d8c72d2 into main Aug 10, 2024
11 checks passed
@mstrasinskis mstrasinskis deleted the mstr/ledger-id branch August 10, 2024 08:42
@mstrasinskis mstrasinskis mentioned this pull request Aug 12, 2024
1 task
github-merge-queue bot pushed a commit to dfinity/nns-dapp that referenced this pull request Aug 12, 2024
# Motivation

To make [getLedgerId api](dfinity/ic-js#693)
available in `nns-dapp` the `ic-js` package needs to be upgraded.

# Changes

- `npm run upgrade:next`.
- Add `[]` to some new optional fields in `convertSwapInitParams` to fix
type checking errors.

# Tests

- Pass

# 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.

3 participants