Skip to content

Upgrading and cleaning up basic_bitcoin example #1135

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

Open
wants to merge 55 commits into
base: master
Choose a base branch
from

Conversation

kristoferlund
Copy link
Contributor

@kristoferlund kristoferlund commented Apr 29, 2025

Summary:

  • Add support for Segwit
  • Upgraded all dependencies to late(st) versions
  • Align naming more clearly with standard terminology
  • Simplify the structure, let functionality hidden in functions within functions bubble up to the service function level. When we share one of these examples with a dev, they should be able to get the full picture without havuing to click down 2-3 levels into subfunctions.
  • Use BIP-32 path derivation instead of custom

256c45b5230b — Upgrading dependencies, cleaning up, first take

  • Remove custom build script as it is not needed
  • Remove rust-toolchain settings, not needed for demo purposes
  • Adding default settings for running bitcoind from project folder
  • Replace custom canister type with rust type to allow for standard dfx deploy
  • Upgrading ic_cdk to v18 and as a consequence, changed a lot of management canister call signatures etc

49bbff34e947 — Upgrade to ic_cdk 18

  • Upgrade bitcoin dependency to 0.32.5
  • Remove bitcoin_api.rs as it just wraps the default bitcoin canister calls -
    introduces abstraction without purpose

92b72a36ea80 — Use management canister api calls instead of "raw" unbounded calls.

  • ic_cdk::call::Call::unbounded_wait(super::mgmt_canister_id(), "sign_with_ecdsa") becomes management_canister::sign_with_ecdsa, etc
  • Replace local schnorr types with the ones defined in ic_cdk

6176865f842e — Rename to align with management canister naming

bac595d71d81 — Storing a global, empty derivation path serves no purpose.

a2f1f45b50ef — "key_name" is a function of network and should not be stored separately

  • Avoid duplication

dab7b45b4de8 — Simplify ecdsa_public_key

  • Cleanup for better readability

0b8917bfce2e — Refactor, remove unnecessary subfolder

  • Simplify naming and imports
  • Remove bitcoin_wallet as it serves no clear purpose. The whole project is about bitcoin wallets.

2744f5d21997 — Cache Schnorr pubkeys

  • Just like the ECDSA keys. Schnorr don't like to be left out.

ff79ff6aa5ad — Add support for Segwit address creation

  • Almost the only addition in this PR.

8cd2bc50aae1 — Refactor: Create individual service functions for better readability and integration with dev docs

  • One service function in each file makes the project easier to navigate. Also makes it easier to reference these service functions in dev docs elsewhere.
  • Also, introduced the concept of BitcoinContext, a struct that is initialised on init and upgrade and can be passed around.
pub struct BitcoinContext {
    network: Network,
    bitcoin_network: bitcoin::Network,
    key_name: &'static str,
}

533be3ff9e18 — Refactor: Naming, comments

c94d23aa0774 — Add support for p2wpkh sending

  • The other half of Segwit support - sending BTC from a Segwit address.

1b841d12aca1 — Refactor: Remove manual sec1_to_der fn, etc

  • Signature provides the built in signature.serialize_der()

c826f4d9d682 — Adding a BIP-32 standard derivation path implementation

Instead of using this very custom derivation scheme:

const P2PKH_DERIVATION_PATH_PREFIX: &str = "p2pkh";
const P2WPKH_DERIVATION_PATH_PREFIX: &str = "p2wpkh";
const P2TR_DERIVATION_PATH_PREFIX: &str = "p2tr_key_and_script_path";
const P2TR_KEY_ONLY_DERIVATION_PATH_PREFIX: &str = "p2tr_key_path_only";

... use standard, BIP-32 derivation paths instead.

3d4355770922 — Refactor Taproot get_* functions for readability etc

  • Align better with standard terminology
  • More comments

da976e270d76 — Exclude canister_ids from commit

9fc8d46a3052 — Refactor send_from_key_path_only_address

  • Align better with standard terminology
  • More comments

0ee09832e655 — Refactor, documentation

790551d631be — README

Remove bitcoin_api.rs as it just wraps
the default bitcoin canister calls -
introduces abstraction without purpose
instead of "raw" unbounded calls.

* Replace local schnorr types with
the ones defined in ic_cdk
@kristoferlund kristoferlund changed the title [WIP] Upgrading dependencies, cleaning up, first take [WIP] Upgrading and cleaning up basic_bitcoin example Apr 30, 2025
@kristoferlund kristoferlund marked this pull request as ready for review May 6, 2025 19:51
@kristoferlund kristoferlund requested a review from a team as a code owner May 6, 2025 19:51
@kristoferlund kristoferlund changed the title [WIP] Upgrading and cleaning up basic_bitcoin example Upgrading and cleaning up basic_bitcoin example May 6, 2025
@kristoferlund kristoferlund requested a review from a team as a code owner May 8, 2025 09:19
kristoferlund and others added 26 commits May 9, 2025 09:55
…led_address_script_spend.rs

Co-authored-by: Jessie Mongeon <[email protected]>
…led_address_script_spend.rs

Co-authored-by: Jessie Mongeon <[email protected]>
…led_address_key_spend.rs

Co-authored-by: Jessie Mongeon <[email protected]>
@letmejustputthishere
Copy link
Member

@dfinity/cross-chain-team FYI

@kristoferlund
Copy link
Contributor Author

@dfinity/execution, this one should be good to go unless you have comments

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