feat(core): optional `shouldAddInputs` for `Transaction.completeFee` #225

Hanssen0 · 2025-06-13T11:42:02Z

No description provided.

changeset-bot · 2025-06-13T11:42:06Z

🦋 Changeset detected

Latest commit: 181c661

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 20 packages

Name	Type
@ckb-ccc/core	Minor
@ckb-ccc/eip6963	Patch
@ckb-ccc/joy-id	Patch
@ckb-ccc/lumos-patches	Patch
@ckb-ccc/nip07	Patch
@ckb-ccc/okx	Patch
@ckb-ccc/rei	Patch
@ckb-ccc/shell	Patch
@ckb-ccc/spore	Patch
@ckb-ccc/ssri	Patch
@ckb-ccc/udt	Patch
@ckb-ccc/uni-sat	Patch
@ckb-ccc/utxo-global	Patch
@ckb-ccc/xverse	Patch
@ckb-ccc/ccc	Patch
ckb-ccc	Patch
@ckb-ccc/connector	Patch
@ckb-ccc/examples	Patch
@ckb-ccc/ccc-playground	Patch
@ckb-ccc/connector-react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Hanssen0 · 2025-06-13T11:43:36Z

@phroi, Please review this pr for #224

Hanssen0 · 2025-06-13T12:25:05Z

The second commit addresses a prettier issue. We can review the first commit only.

phroi · 2025-06-13T13:52:27Z

packages/core/src/ckb/transactionErrors.ts

Perfect, I wanted exactly to propose to move to this kind of specific errors for insufficient balance.

One note: would it make sense to also add the Coin name or symbol as in the error? (both message and property)

We need a data source for name & symbol (SSRI/indexer), which will introduce extra cost and complexity, so I don't think it is worth it.

I suggest we can add the type to the error property. As for the message, that will cause problems with compatibility and readability, so I prefer to keep it as it is.

Updated to include the type in Error properties.

Reasonable! I also noticed that now the properties are required, nice!! 👍

phroi · 2025-06-13T13:57:38Z

packages/core/src/ckb/transaction.ts

Perfect, in general I'd love to see more TypeDoc of this kind!! 👍 (Specific and descriptive)

With LLMs these TypeDoc are really easy to generate and often times they provide a lot of information to a new developer that otherwise would have to read thru the code itself instead of its documentation, so it could make sense to make it as best practice

Agree. AI generates these docs and helps. We will add more docs in the future.

phroi · 2025-06-13T14:22:04Z

packages/core/src/ckb/transaction.ts

    let leastExtraCapacity = Zero;

    while (true) {
      const tx = this.clone();


There is an issue right here: clone doesn't copy input metadata, so we are still re-fetching every single cell.

Let's say the current cell pool being examined is bigger that than the cache size, the cache will be thrashed and these requests will spill out to the underlying RPC. (Even worse if the Client doesn't have a cache)

While this is out of the scope of this FR, it could make sense to let the offline check operate on the original transaction, not on the clone.

What's your take on this @Hanssen0?

Also, in general, what's preventing us from cloning and prevent re-fetching in general by then shallowly copying the inputs?

Thank you for noticing! This is a mistake. Once in a while, the clone was implemented manually, and the cache in the CellInput will be handled appropriately. Later, we change this to the default implementation from molecule, which doesn't handle these extra fields.

I will fix this.

it could make sense to let the offline check operate on the original transaction, not on the clone.

For this, because the change function modifies the transaction, but we need to do this multiple times on the original transaction, we must clone the transaction to preserve the original version.

Glad to hear!! Same as the core the the FR, this has been bothering me for a long time now, so much that I modified the behaviour of SmartTransaction.clone to work around it! 🤣 (My solution was to shallowly copying inputs right in clone, you may want to consider this behaviour)

The new fix commit should fix this problem. Please check.

The CellInput is deep cloned to avoid any future problems. Although change usually won't modify the inputs, we have no idea what others may do with the cloned Transaction.

phroi · 2025-06-13T14:51:17Z

packages/core/src/ckb/transaction.ts

+      this.version,
+      this.cellDeps.map((c) => c.clone()),
+      this.headerDeps.map((h) => h),
+      this.inputs.map((i) => i.clone()),


Nice!! 👍

phroi · 2025-06-13T15:08:35Z

There is another smaller issue: completeFee throws multiple times for insufficient capacity, but only one of these error is categorized in the appropriate Error ErrorTransactionInsufficientCapacity.

In this regard, completeFee code seems to be choosing Error message vocalization over error recoverability.

What's your take on this @Hanssen0?

phroi · 2025-06-13T15:18:37Z

packages/core/src/ckb/transaction.ts

    while (true) {
      const tx = this.clone();
      const collected = await (async () => {
+        if (!(options?.shouldAddInputs ?? true)) {


Smaller issue: why is this check inside the loop and not before the loop?

Inside the loop means it will be checked at every iteration, before the loop means that exactly one check happens for a single completeFee call.

What's your take @Hanssen0?

It's because the leastFee and leastExtraCapacity change every iteration. If the change function requests more capacity for the change cell, the check might fail.

You are indeed correct 👍

ashuralyk

I see no problems in the main change area, aka shouldAddInputs option, from my point of view.

Hanssen0 · 2025-06-13T15:23:35Z

There is another smaller issue: completeFee throws multiple times for insufficient capacity, but only one of these error is categorized in the appropriate Error ErrorTransactionInsufficientCapacity.

In this regard, completeFee code seems to be choosing Error message vocalization over error recoverability.

What's your take on this @Hanssen0?

Normally, errors related to the change function should not be thrown if it's implemented correctly. I think the only confusing part is the Not enough capacity for the change cell error. We can keep the other two as they are, since they're similar to panic.

Indeed, when I was designing this part, I chose vocalization to help identify the problem without adding too much work. I still think ErrorTransactionInsufficientCapacity is different from ErrorTransactionInsuffcientCapacityForChange, but maybe it's the right time to make it a typed Error now.

How do you think about adding another Error type for the change error?

phroi · 2025-06-13T15:31:33Z

I still think ErrorTransactionInsufficientCapacity is different from ErrorTransactionInsuffcientCapacityForChange, but maybe it's the right time to make it a typed Error now.

I would make it ErrorTransactionInsufficientCapacity:

Add a bool property to denote if it's due to change cell
Error message should change depending on this bool, potentially appending to the common general message a postfix string indicating the specificity of the error (for change cell)

phroi · 2025-06-13T15:35:54Z

Having a unified error could simplify error handling for the final dev. At any rate, you know best, so it's your call

Hanssen0 · 2025-06-13T15:38:36Z

Error message should change depending on this bool, potentially appending to the common general message a postfix string indicating the specificity of the error (for change cell)

It sounds good. It's just a difficult decision for me to break the compatibility.

I will follow this approach to simplify error handling and hope that no one will be affected by this (well, it should not be a big problem, right?).

phroi · 2025-06-13T15:45:52Z

Fair point! I believe that if they are checking the error based on prefix, by using the more general prefix, the error will be still ok.

An alternative is to use a totally different message in case of change cell for being back-ward compatible 🤔 (still within the context of having it as bool/string-type property in ErrorTransactionInsufficientCapacity)

Hanssen0 · 2025-06-13T15:46:18Z

Error message should change depending on this bool, potentially appending to the common general message a postfix string indicating the specificity of the error (for change cell)

Updated the feat commit.

phroi · 2025-06-13T15:48:09Z

packages/core/src/ckb/transactionErrors.ts

+    const amount = numFrom(amountLike);
+    const isForChange = reason?.isForChange ?? false;
+    super(
+      `Insufficient CKB, need ${fixedPointToString(amount)} extra CKB${isForChange ? " for the change cell" : ""}`,


Nice!! 👍

phroi

⭐️⭐️⭐️⭐️ (5/5)

I submitted all the issues I slowly found out over months: they have been analyzed in dept and solved brilliantly in the blink of an eye, while also minimizing compatibility breakage. Excellent quality!! 👍

Definitely recommended, I will hang around here again!

Phroi %93 😁

Hanssen0 · 2025-06-13T16:15:11Z

⭐️⭐️⭐️⭐️ (5/5)

I submitted all the issues I slowly found out over months: they have been analyzed in dept and solved brilliantly in the blink of an eye, while also minimizing compatibility breakage. Excellent quality!! 👍

Definitely recommended, I will hang around here again!

Phroi 😁

Thank you for all the contributions! Without you, we wouldn't be able to keep making this better and better. Together, we will make development in the UTXO ecosystem easier.

Hanssen0 · 2025-06-13T16:32:09Z

A canary version has been published for us to test:
https://github.com/ckb-devrel/ccc/actions/runs/15639378393

Feel free to open an issue for any problems! We will publish the next stable version as soon as it's ready.

Hanssen0 force-pushed the feat/complete-fee-no-new-cells branch 2 times, most recently from d429bd9 to 59afacc Compare June 13, 2025 12:23

phroi reviewed Jun 13, 2025

View reviewed changes

Hanssen0 force-pushed the feat/complete-fee-no-new-cells branch from 59afacc to 3141935 Compare June 13, 2025 14:07

phroi reviewed Jun 13, 2025

View reviewed changes

Hanssen0 force-pushed the feat/complete-fee-no-new-cells branch 2 times, most recently from 3d1f59d to d993dd3 Compare June 13, 2025 14:45

phroi reviewed Jun 13, 2025

View reviewed changes

ashuralyk approved these changes Jun 13, 2025

View reviewed changes

Hanssen0 added 3 commits June 13, 2025 23:45

feat(core): optional shouldAddInputs for Transaction.completeFee

33a1dde

fix(core): Transaction.clone should clone inputs' cache

ce3a227

chore: ignore CHANGELOG.md for prettier

181c661

Hanssen0 force-pushed the feat/complete-fee-no-new-cells branch from d993dd3 to 181c661 Compare June 13, 2025 15:45

phroi reviewed Jun 13, 2025

View reviewed changes

phroi approved these changes Jun 13, 2025

View reviewed changes

Hanssen0 merged commit 813319a into ckb-devrel:master Jun 13, 2025
1 check passed

Hanssen0 deleted the feat/complete-fee-no-new-cells branch June 13, 2025 16:20

This was referenced Jun 13, 2025

chore(master): bump packages version #226

Closed

chore(master): bump packages version Hanssen0/ccc#8

Open

feat(core): optional shouldAddInputs for Transaction.completeFee #225

feat(core): optional shouldAddInputs for Transaction.completeFee #225

Uh oh!

Conversation

Hanssen0 commented Jun 13, 2025

Uh oh!

changeset-bot bot commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Hanssen0 commented Jun 13, 2025

Uh oh!

Hanssen0 commented Jun 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phroi Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phroi commented Jun 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashuralyk left a comment

Choose a reason for hiding this comment

Uh oh!

Hanssen0 commented Jun 13, 2025

Uh oh!

phroi commented Jun 13, 2025

Uh oh!

phroi commented Jun 13, 2025

Uh oh!

Hanssen0 commented Jun 13, 2025

Uh oh!

phroi commented Jun 13, 2025

Uh oh!

Hanssen0 commented Jun 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phroi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hanssen0 commented Jun 13, 2025

Uh oh!

Uh oh!

Hanssen0 commented Jun 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

feat(core): optional `shouldAddInputs` for `Transaction.completeFee` #225

feat(core): optional `shouldAddInputs` for `Transaction.completeFee` #225

changeset-bot bot commented Jun 13, 2025 •

edited

Loading

phroi Jun 13, 2025 •

edited

Loading

phroi left a comment •

edited

Loading