Skip to content

add BIP341 to Motoko basic_bitcoin #1080

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

Merged
merged 9 commits into from
Jan 15, 2025

Conversation

altkdf
Copy link
Contributor

@altkdf altkdf commented Jan 10, 2025

No description provided.

/// Returns the ECDSA public key of this canister at the given derivation path.
public func ecdsa_public_key(key_name : Text, derivation_path : [Blob]) : async Blob {
public func ecdsa_public_key(ecdsa_canister_actor: EcdsaCanisterActor, key_name : Text, derivation_path : [Blob]) : async Blob {
Copy link
Contributor Author

@altkdf altkdf Jan 10, 2025

Choose a reason for hiding this comment

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

Since dfx/ pocket IC doesn't support key tweaking yet, we need to use the mock canister. Previously, the actor was defined as actor ("aaaaa-aa") in the module, but now we need to change it dynamically based on the canister API, but Motoko doesn't allow for non-static variables in modules. Therefore, defining EcdsaCanisterActor in the top-level actor and passing it down as an argument is the best way to make it work that I could find.

Copy link
Member

Choose a reason for hiding this comment

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

Do I understand correctly that once dfx+pocketIC support key tweaking, we don't need to pass the actor as argument any more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessarily. It seems useful for testing to be able to change the management canister actor easily.

@altkdf
Copy link
Contributor Author

altkdf commented Jan 10, 2025

Motoko style actually uses camel case for variables and function names, but I will leave this change for another PR...

@altkdf altkdf requested review from randombit and fspreiss January 10, 2025 10:41
@altkdf altkdf marked this pull request as ready for review January 10, 2025 10:43
@altkdf altkdf requested review from a team as code owners January 10, 2025 10:43
fspreiss
fspreiss previously approved these changes Jan 13, 2025
Copy link
Member

@fspreiss fspreiss left a comment

Choose a reason for hiding this comment

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

Thanks, @altkdf! LGTM, not an expert on the topic, however. Quite some code was moved to new files: I only glanced over this code and didn't do an in-depth review.

/// Returns the ECDSA public key of this canister at the given derivation path.
public func ecdsa_public_key(key_name : Text, derivation_path : [Blob]) : async Blob {
public func ecdsa_public_key(ecdsa_canister_actor: EcdsaCanisterActor, key_name : Text, derivation_path : [Blob]) : async Blob {
Copy link
Member

Choose a reason for hiding this comment

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

Do I understand correctly that once dfx+pocketIC support key tweaking, we don't need to pass the actor as argument any more?

@altkdf
Copy link
Contributor Author

altkdf commented Jan 13, 2025

It would also make sense if someone except me would check that this example works with bitcoind because there are no automated tests.

Copy link
Contributor

@berestovskyy berestovskyy left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@altkdf altkdf merged commit da45b63 into master Jan 15, 2025
5 checks passed
@altkdf altkdf deleted the alex/add-bip341-to-motoko-basic-bitcoin branch January 15, 2025 13:32
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