Skip to content

refactor(ckbtc): Clean up types used by ckbtc minter #4757

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 17 commits into
base: master
Choose a base branch
from

Conversation

ninegua
Copy link
Member

@ninegua ninegua commented Apr 10, 2025

We have a few types defined in both ic_btc_interface and ic_sdk, with mostly similar and slightly different implementations. It is confusing to use both of them in the minter, and it is inconvenient because we can't implement From instances directly.

This PR introduces some cleanups as explained below:

  1. Define our own Network type (by renaming lifecycle::init::BtcNetwork to crate::Network):
  • do not use the one from ic_btc_interface because it is not required anywhere
  • do not use the one from ic_cdk directly because it does not implement Display
  • convert to the one from ic_cdk because the latter is required to make management calls
  1. Define GetUtxosRequest as a type synonym of the one from ic_cdk:
  • so that it will be easier to extend with extra fields (not in this PR)
  • do not use the one from ic_btc_interface because it is not required anywhere
  1. Define our own GetUtxosResponse type:
  • do not use the one from ic_btc_interface because it is not required anywhere
  • do not use the one from ic_cdk directly because we need Utxo type from ic_btc_interface
  • convert from the one from ic_cdk because the latter is returned from management calls

@ninegua ninegua requested a review from a team as a code owner April 10, 2025 08:16
@gregorydemay gregorydemay added the CI_ALL_BAZEL_TARGETS Runs all bazel targets and uploads them to S3 label Apr 10, 2025
Copy link
Member

@gregorydemay gregorydemay left a comment

Choose a reason for hiding this comment

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

Thanks @ninegua for this clean-up! Some understanding question from my side but otherwise code looks good!

Copy link
Member

@gregorydemay gregorydemay left a comment

Choose a reason for hiding this comment

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

Thanks @ninegua for the changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI_ALL_BAZEL_TARGETS Runs all bazel targets and uploads them to S3 @cross-chain-team refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants