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

chore(ckbtc): remove distribute_kyt_fee and reimburse_failed_kyt #3325

Merged
merged 2 commits into from
Jan 11, 2025

Conversation

ninegua
Copy link
Member

@ninegua ninegua commented Jan 6, 2025

XC-237

Since the ckBTC minter switched to using the checker canister, it no longer needs to distribute kyt fees, or reimburse failed kyt. These calls can be removed from the code, but we do still need to keep them for event and state handling during replay.

@github-actions github-actions bot added the chore label Jan 6, 2025
@ninegua ninegua force-pushed the paulliu/remove-distribute_kyt_fee branch from e759059 to 7d3f7ac Compare January 6, 2025 02:20
@ninegua ninegua added the CI_ALL_BAZEL_TARGETS Runs all bazel targets and uploads them to S3 label Jan 7, 2025
@ninegua ninegua force-pushed the paulliu/remove-distribute_kyt_fee branch from 7d3f7ac to 9e0c23e Compare January 7, 2025 00:45
@ninegua ninegua marked this pull request as ready for review January 8, 2025 14:10
@ninegua ninegua requested a review from a team as a code owner January 8, 2025 14:10
Copy link
Contributor

@lpahlavi lpahlavi left a comment

Choose a reason for hiding this comment

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

Just a small nit: could we deprecate the DistributedKytFee event?

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 PR! I have the impression we can delete more code 😄

rs/bitcoin/ckbtc/minter/src/main.rs Show resolved Hide resolved
rs/bitcoin/ckbtc/minter/src/main.rs Show resolved Hide resolved
rs/bitcoin/ckbtc/minter/src/lib.rs Show resolved Hide resolved
rs/bitcoin/ckbtc/minter/src/lib.rs Show resolved Hide resolved
@ninegua
Copy link
Member Author

ninegua commented Jan 9, 2025

Just a small nit: could we deprecate the DistributedKytFee event?

I believe this event type is part of the historical events, which are still used during an event playback (e.g, when upgrading the minter). So the code handling them will need to be kept around.

@lpahlavi
Copy link
Contributor

Just a small nit: could we deprecate the DistributedKytFee event?

I believe this event type is part of the historical events, which are still used during an event playback (e.g, when upgrading the minter). So the code handling them will need to be kept around.

Ah yes, I read this too quickly. I thought I remembered that the code replaying events allowed the use of deprecated events for backwards compatibility. Thanks for the clarification!

@ninegua ninegua added this pull request to the merge queue Jan 11, 2025
Merged via the queue into master with commit cfd1859 Jan 11, 2025
25 checks passed
@ninegua ninegua deleted the paulliu/remove-distribute_kyt_fee branch January 11, 2025 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore CI_ALL_BAZEL_TARGETS Runs all bazel targets and uploads them to S3 @cross-chain-team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants