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

System API for threshold ECDSA #6

Merged
merged 13 commits into from
Apr 12, 2022
Merged

Conversation

ninegua
Copy link
Member

@ninegua ninegua commented Jan 25, 2022

We propose two new system APIs for the upcoming threshold ECDSA feature in the management canister.

@ninegua ninegua force-pushed the paul/CON-742-tecdsa-system-api branch from 77a4932 to 3faba31 Compare January 25, 2022 18:26
@ninegua ninegua force-pushed the paul/CON-742-tecdsa-system-api branch from 3faba31 to ebf0905 Compare January 25, 2022 18:27
@ninegua
Copy link
Member Author

ninegua commented Jan 25, 2022

Points for discussion:

  1. Support additional parameters like key type / curve, key identifier (which subnet key), etc.

These can be added later by adding optional fields to the input. It will be backward compatible as long as the new fields have sensible defaults.

  1. Support more elaborate error message.

At the moment all errors are delivered through reject messages instead of returning a variant type like Result.

  1. Whether / how the derivation_path can be made compliant to BIP32.

Copy link
Member

@manudrijvers manudrijvers left a comment

Choose a reason for hiding this comment

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

comments from the ecdsa sync meeting

spec/ic.did Outdated Show resolved Hide resolved
spec/ic.did Outdated Show resolved Hide resolved
@ninegua ninegua marked this pull request as ready for review February 4, 2022 19:54
spec/index.adoc Outdated Show resolved Hide resolved
spec/ic.did Outdated Show resolved Hide resolved
spec/index.adoc Outdated Show resolved Hide resolved
spec/index.adoc Outdated Show resolved Hide resolved
Copy link
Contributor

@andreacerulli andreacerulli left a comment

Choose a reason for hiding this comment

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

Thanks a lot @ninegua! I left some suggestions and nits, but otherwise it looks good.

spec/index.adoc Outdated Show resolved Hide resolved
spec/index.adoc Outdated Show resolved Hide resolved
spec/index.adoc Outdated Show resolved Hide resolved
spec/index.adoc Show resolved Hide resolved
spec/index.adoc Outdated Show resolved Hide resolved
spec/index.adoc Outdated Show resolved Hide resolved
spec/index.adoc Show resolved Hide resolved
spec/ic.did Outdated Show resolved Hide resolved
spec/index.adoc Outdated Show resolved Hide resolved
@ninegua ninegua force-pushed the paul/CON-742-tecdsa-system-api branch from ef389c4 to aa19fc0 Compare March 10, 2022 16:11
@ninegua ninegua requested a review from a team as a code owner April 6, 2022 20:43
@ninegua ninegua force-pushed the paul/CON-742-tecdsa-system-api branch from 07d1bd5 to d7b49e2 Compare April 6, 2022 20:46
Copy link
Member

@Dfinity-Bjoern Dfinity-Bjoern left a comment

Choose a reason for hiding this comment

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

Thanks for the careful consideration of all discussions and feedback!

@Ali-Piccioni Ali-Piccioni reopened this Apr 12, 2022
@ninegua ninegua merged commit a6b7417 into master Apr 12, 2022
@ninegua ninegua deleted the paul/CON-742-tecdsa-system-api branch April 12, 2022 21:01
@nomeata
Copy link
Contributor

nomeata commented Apr 12, 2022

Did we intentionally switch to non-squash merges now, or is that simply not discussed yet?

(I'd be in favor of re-enabling mergify; automatic squash merge with PR-description-as-commit-message seems to be the simplest nicest option.)

dfinity-bot pushed a commit to dfinity/ic that referenced this pull request Apr 21, 2022
EXC-1066: Enhance ECDSA key id type

Change the type of the ECDSA keys from a string to a struct consisting
of the curve (an enum) and the name (a string). Per the spec
dfinity/interface-spec#6 (comment) 

See merge request dfinity-lab/public/ic!4344
basvandijk pushed a commit to dfinity/ic-hs that referenced this pull request May 30, 2022
This PR Implements a corresponding part of the IC spec: dfinity/interface-spec#6

It also splits up `IC.Test.Agent` and `IC.Test.Spec` to reduce the excessive memory consumption of compiling those modules.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.