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

feature: add API endpoint 'ecdsa_public_key' to ic-management #652

Closed

Conversation

AntonioVentilii-DFINITY
Copy link

Motivation

As first step, before including a function to generate a P2PKH address, we add the usage of IC method ecdsa_public_key.

Changes

  • Created types for parameters and response of API endpoint ecdsa_public_key, adapted to the current package.
  • Created function ecdsaPublicKey in ic-management module.
  • Created tests for ecdsaPublicKey.
  • Included usage in README file.

Tests

Created a new test specific to the usage of ecdsaPublicKey.

Todos

  • Add entry to changelog (if approved).

Copy link
Contributor

github-actions bot commented Jun 7, 2024

size-limit report 📦

Path Size
@dfinity/ckbtc 7.89 KB (0%)
@dfinity/cketh 3.49 KB (0%)
@dfinity/cmc 1.29 KB (0%)
@dfinity/ledger-icrc 3.64 KB (0%)
@dfinity/ledger-icp 15.22 KB (0%)
@dfinity/nns 34.62 KB (0%)
@dfinity/nns-proto 140.98 KB (0%)
@dfinity/sns 15.72 KB (0%)
@dfinity/utils 4.47 KB (0%)
@dfinity/ic-management 2.8 KB (+1.42% 🔺)

@AntonioVentilii-DFINITY AntonioVentilii-DFINITY marked this pull request as ready for review June 7, 2024 19:06

##### :gear: ecdsaPublicKey

Calculate a SEC1 encoded ECDSA public key for the given canister using the given derivation path. If the `canister_id` is unspecified, it will default to the canister id of the caller. The `derivation_path` is a vector of variable length byte strings. Each byte string may be of arbitrary length, including empty. The total number of byte strings in the `derivation_path` must be at most 255. The `key_id` is a struct specifying both a `curve` and a `name`. The availability of a particular `key_id` depends on implementation.
Copy link
Member

Choose a reason for hiding this comment

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

If the canister_id is unspecified, it will default to the canister id of the caller.

Wait, are you sure this endpoint can be called from a frontend client?
Or is this description incorrect?

I don't see how the canister can be identified if the calls happen on the cllient side.

Choose a reason for hiding this comment

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

Correct, it can be called without canister ID parameter and it will use the principal of the initialized service (the caller), that according to the code is defined during the creation of the service:

const { service } = createServices<IcManagementService>({
    options: {
      ...options,
      // Resolve to "aaaaa-aa" on mainnet
      canisterId: Principal.fromHex(""),
      callTransform: transform,
      queryTransform: transform,
    },
    idlFactory,
    certifiedIdlFactory,
  });

If you prefer, we can make canisterId non-optional for the ic-js scope, but the original call to the API accepts this behavior, if you want to keep consistency (see function provisionalCreateCanisterWithCycles in the same module).

Copy link
Member

Choose a reason for hiding this comment

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

The documentation states the following:

If the canister_id is unspecified, it will default to the canister id of the caller.

So, assuming it's just misleading, I would review the documentation.

Then, regarding your answer.

Correct, it can be called without canister ID parameter and it will use the principal of the initialized service

Do you mean that it will use the principal of your snippet canisterId: Principal.fromHex("")? If so, I doubt that's correct or, again it's a misunderstanding and the documentation should state that if no parameter canisterId is provided, the address will be deferred from the canisterId on which the service lies - i.e. the Bitcoin canisterId.

Copy link
Author

Choose a reason for hiding this comment

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

Agree, it is misleading. Check the new description, let me know if it has improved.

packages/ic-management/README.md Outdated Show resolved Hide resolved
packages/ic-management/src/types/ic-management.params.ts Outdated Show resolved Hide resolved
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.

The PR should not be merged yet. We are currently reviewing the feature as it appears that the call can most likely only be performed by a canister and therefore cannot be used on the client side.

@AntonioVentilii-DFINITY
Copy link
Author

After discussing with the Execution Team, I found out that the endpoint ecdsa_public_key will not work from front-end directly. So, this PR is closed.

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.

None yet

2 participants