-
Notifications
You must be signed in to change notification settings - Fork 22
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
fix: handle edge case when change amount is below minimum capacity #313
base: develop
Are you sure you want to change the base?
Conversation
@@ -382,18 +382,30 @@ export const appendIssuerCellToBtcBatchTransferToSign = async ({ | |||
|
|||
let actualInputsCapacity = BigInt(sumInputsCapacity); | |||
const txFee = MAX_FEE; | |||
if (actualInputsCapacity <= sumOutputsCapacity) { | |||
let changeCapacity = actualInputsCapacity - sumOutputsCapacity; | |||
while (changeCapacity < MIN_CAPACITY) { |
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.
Why do you use a while
loop? How do you return from the loop?
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 think if the changeCapacity < MIN_CAPACITY
is true, throwing an error is enough for issuers.
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.
Initially I mistook txFee
for fee rate - I thought the loop ensures sufficient capacity to cover increasing transaction fees as the transaction size grows with each new input from collectInputs
. As actualInputsCapacity
accumulates in the loop, changeCapacity
gradually increases until it either exceeds MIN_CAPACITY
for normal exit, or throws CapacityNotEnoughError
when available cells are insufficient.
Upon closer inspection after reading your comments, I realize a while
loop is unnecessary since txFee
is set to a sufficiently large MAX_FEE
constant. A simple if
statement would be adequate here.
Despite its name, appendIssuerCellToBtcBatchTransferToSign
serves as a general fee supplement method in offline mode. Perhaps we should collect additional issuer's cells as inputs to cover tx fee whenever possible, instead of throwing an error. Please let me know your thoughts.
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
MAX_FEE
is 0.2CKB, and it is enough for almost transactions. - If the issuer's balance is not enough for collecting, the loop does not work.
- I think you could provide a capacity estimate function for the issuer before collecting cells.
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.
Good point! I'll try to implement it later.
When
actualInputsCapacity > sumOutputsCapacity
but the resulting change amount is belowMIN_CAPACITY
, we also need to collect additional cells to ensure the change output meets the minimum capacity requirement. This prevents creation of invalid cells with insufficient capacity.