-
Notifications
You must be signed in to change notification settings - Fork 372
fix(ckdoge): reject deposits below minimum amount #8428
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
base: master
Are you sure you want to change the base?
Conversation
|
|
||
| for utxo in processable_utxos { | ||
| if utxo.value <= check_fee { | ||
| let ignored_reason = if utxo.value < deposit_btc_min_amount { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: One would expect that a deposit of exactly deposit_btc_min_amount is permissible because the name suggests that it is the minimum accepted value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get it, a deposit of exactly deposit_btc_min_amount would be accepted, since only if the utxo value is strictly less than the required minimum amount would it be ignored.
if utxo.value < deposit_btc_min_amount| ecdsa_key_name : text; | ||
|
|
||
| // The minimal amount of BTC that can be converted to ckBTC. | ||
| deposit_btc_min_amount : opt nat64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the advantage of having a concrete parameter?
Why not simply have effective_deposit_min_btc_amount return max(MINTER_ADDRESS_P2WPKH_DUST_LIMIT, check_fee+1)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the advantage of having a concrete parameter?
- There will be at least 2 different values used, e.g.
MINTER_ADDRESS_P2WPKH_DUST_LIMITfor ckBTC and 1 DOGE for ckDOGE (an instance ofCkBtcMinterStateis also used by the ckDOGE minter). - We could use hard-coded constants that we put somewhere, e.g.
CanisterRuntime, but to me it makes more sense to have it as init/upgrade args. It's easier to track what's going on in proposals and this also allows to have potentially different values between the different environments (ckStagingDOGE or ckTESTBTC)
ninegua
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM
mducroux
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! LGTM
|
|
||
| // The upgrade parameters of the minter canister. | ||
| type UpgradeArgs = record { | ||
| // The minimal amount of BTC that can be converted to ckBTC. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc does not make it clear if this is about the minimum amount in a single tx or in a single UTXO.
Ensure that DOGE deposits will be ignored when their value is below a configurable minimum amount. This is needed to ensure that the minter does not control UTXOs whose value are below the dust threshold (0.01 DOGE). This was not a problem for ckBTC, which deducts a
check_feeon the deposited UTXO value and whose value is above the Bitcoin dust threshold. In contrast, ckDOGE does not deduct a checker fee (check_feeis0), but still requires deposited UTXOs to have sufficient value.For that reason, the following changes were made to the ckBTC and ckDOGE minter:
deposit_btc_min_amount, which controls the minimum value a deposited UTXO must have to avoid being ignored.check_feeanddeposit_btc_min_amount) is returned as part of theMinterInfoand displayed on the minter dashboard.About
CI_OVERRIDE_DIDC_CHECK: the following non-breaking changes were made to the Candid interface:ckbtc_minter.didInitArgsandUpgradeArgsMinterInfockdoge_minter.did: the ckDOGE minter is not yet live, so that any change here is by definition not breaking.