feat(rgbpp-ckb): build btc transfer virtual ckb tx #13

duanyytop · 2024-03-07T09:36:28Z

Changes

Add BTC transfer virtual CKB tx builder
Add necessary methods for service to build and sign CKB transactions
Add necessary utils methods and RGB++ types

Reviewers

@ShookLyngs @ahonn @Flouse

Flouse · 2024-03-07T13:10:18Z

Add layer1 transfer virtual tx builder

In the design documentation, BTC is referred to as Layer 1 (L1).

To avoid conceptual confusion, I suggest:

avoid using L1, L2 naming, just use BTC_TX and CKB_TX style, which is clearer.
e.g. L1VirtualTx -> ckbVirtualTx

packages/ckb/src/utils/rgbpp.ts

Co-authored-by: Flouse <[email protected]>

duanyytop · 2024-03-07T14:24:54Z

I will add more test cases when the test cases of RGBPP lock script are finished

Flouse · 2024-03-07T15:00:32Z

packages/ckb/src/collector/index.ts

+    if (sumAmount < needAmount) {
+      throw new UdtAmountNotEnoughError('Insufficient UDT balance');
+    }
+    return { inputs, capacity: sumCapacity, amount: sumAmount };


suggestion:
update https://github.com/ckb-cell/rgbpp-sdk/blob/0d18e28e586f4599a870de414bfe5fd7cd745a71/packages/ckb/src/types/collector.ts#L22-L25

Updated ec07a01

packages/ckb/src/rgbpp/ckb-builder.ts

Flouse · 2024-03-07T17:00:08Z

packages/ckb/src/rgbpp/ckb-builder.ts

+  const transactionHash = rawTransactionToHash(rawTx);
+  const signedWitnesses = signWitnesses(keyMap)({
+    transactionHash,
+    witnesses: rawTx.witnesses,
+    inputCells: cells,
+    skipMissingKeys: true,
+  });
+  const emptyWitness = { lock: '', inputType: '', outputType: '' };
+  const signedTx = {
+    ...rawTx,
+    witnesses: signedWitnesses.map((witness, index) => (index === 0 ? serializeWitnessArgs(emptyWitness) : witness)),
+  };
+  return signedTx;
+};


need test cases later.

Yes. The integration test will be added later and I think we should focus on the new features now.

ShookLyngs · 2024-03-07T17:11:21Z

.github/workflows/test.yaml

@@ -42,7 +42,7 @@ jobs:

      - name: Install dependencies
        run: pnpm i
-
+        


The test workflow doesn't seem to have any significant changes?

I think adding an empty line will be more clear and I don't think it's a bad thing. Do you think so ?

If you think it's necessary to remove the empty line, I will do it.

Yes, I agree with you. I think breaking steps with an empty line is clearer than collapsing them together. However, I noticed that the only change in the test.yaml file in the PR is the addition of more spaces on line 45, rather than adding more line breaks as you suggested.

The test.yaml file in the main branch: https://github.com/ckb-cell/rgbpp-sdk/blob/f67460f3e4a2cc1620ea1a59baaa14bc78b51d32/.github/workflows/test.yaml#L43-L47

The test.yaml in the current PR: https://github.com/ckb-cell/rgbpp-sdk/blob/dabbb8a4346c30b1416437a13ac3f412cebfce04/.github/workflows/test.yaml#L43-L47

ShookLyngs · 2024-03-08T02:29:33Z

If the PR contains noticeable or significant changes, please include a changeset in each relevant commit (1-to-1).
You can run the changeset command locally to create a changeset, which should look like this: b376000#diff-b8615fc591e5d8f2fa98f08cfc873a2c9328fd4d2ef1c55f0d8ed9d63ca3d32a

duanyytop · 2024-03-08T02:35:28Z

If the PR contains noticeable or significant changes, please include a changeset in each relevant commit (1-to-1). You can run the changeset command locally to create a changeset, which should look like this: b376000#diff-b8615fc591e5d8f2fa98f08cfc873a2c9328fd4d2ef1c55f0d8ed9d63ca3d32a

We don’t have enough time now, and there are still many features that need to be implemented. I don’t think adding the changeset which only includes the PR's name is necessary. What do you think？ @Flouse

Flouse · 2024-03-08T15:49:32Z

If the PR contains noticeable or significant changes, please include a changeset in each relevant commit (1-to-1). You can run the changeset command locally to create a changeset, which should look like this: b376000#diff-b8615fc591e5d8f2fa98f08cfc873a2c9328fd4d2ef1c55f0d8ed9d63ca3d32a

We don’t have enough time now, and there are still many features that need to be implemented. I don’t think adding the changeset which only includes the PR's name is necessary. What do you think？ @Flouse

Yes, we can follow @ShookLyngs 's suggestion once we are ready to open source.
IMO, for now, a clear PR name and description is enough.

feat(rgbpp-sdk/service): sync apis from btc-assets-api #13 and #21

duanyytop requested a review from ShookLyngs March 7, 2024 09:36

duanyytop added 14 commits March 7, 2024 17:45

add rgbpp transfer type

5df87f5

add rgbpp types

d49c0b5

add collector methods

8fbe2fa

update secp256k1 constants

7f9c9ad

add ckb tx and rpbpp utils

d429b07

add custom errors

70a15f6

feat: add L1 transfer constructor

aa04831

feat: add appendPaymasterCell for sevice

2905812

feat: add signRgbppTx for service

80bd6c4

refactor: update appendPaymasterCellAndSignTx

19efef4

refacotr: update builder types

5c7ceb3

add sendCkbTx for service

dc35769

chore: update github actions

e1f581b

add github actions lint

39fc5b9

duanyytop force-pushed the l1-transfer-virtual branch from 8109ab3 to 052be60 Compare March 7, 2024 09:47

fix main rebase conflict

ffc7a21

duanyytop force-pushed the l1-transfer-virtual branch from 052be60 to ffc7a21 Compare March 7, 2024 09:52

Flouse requested changes Mar 7, 2024

View reviewed changes

packages/ckb/src/utils/rgbpp.ts Outdated Show resolved Hide resolved

duanyytop and others added 2 commits March 7, 2024 22:09

Update packages/ckb/src/utils/rgbpp.ts

bc7886e

Co-authored-by: Flouse <[email protected]>

refactor: update l1/l2 to btc/ckb

0d18e28

duanyytop changed the title ~~build layer1 transfer virtual tx~~ build btc transfer virtual ckb tx Mar 7, 2024

duanyytop requested a review from Flouse March 7, 2024 14:21

Flouse reviewed Mar 7, 2024

View reviewed changes

Flouse self-requested a review March 7, 2024 15:58

Flouse reviewed Mar 7, 2024

View reviewed changes

ShookLyngs requested changes Mar 7, 2024

View reviewed changes

refactor: Update collector types

ec07a01

duanyytop added 3 commits March 8, 2024 09:36

update multi input locks networks

d864f35

update calculateCommitment

e586c4f

add calculateCommitment test

0057411

duanyytop requested a review from Flouse March 8, 2024 02:03

add hex utils tests

dabbb8a

Flouse approved these changes Mar 8, 2024

View reviewed changes

Flouse changed the title ~~build btc transfer virtual ckb tx~~ feat(rgbpp-ckb): build btc transfer virtual ckb tx Mar 8, 2024

duanyytop merged commit bcfc3fa into main Mar 9, 2024
1 check passed

duanyytop deleted the l1-transfer-virtual branch March 9, 2024 00:26

Flouse added a commit that referenced this pull request Apr 3, 2024

Merge pull request #96 from ckb-cell/feat/paymaster-info-api

cf25ea0

feat(rgbpp-sdk/service): sync apis from btc-assets-api #13 and #21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rgbpp-ckb): build btc transfer virtual ckb tx #13

feat(rgbpp-ckb): build btc transfer virtual ckb tx #13

duanyytop commented Mar 7, 2024 •

edited

Loading

Flouse commented Mar 7, 2024 •

edited

Loading

duanyytop commented Mar 7, 2024

Flouse Mar 7, 2024 •

edited

Loading

duanyytop Mar 8, 2024

Flouse Mar 7, 2024

duanyytop Mar 8, 2024 •

edited

Loading

Flouse Mar 8, 2024

ShookLyngs Mar 7, 2024

duanyytop Mar 8, 2024

ShookLyngs Mar 8, 2024

ShookLyngs commented Mar 8, 2024 •

edited

Loading

duanyytop commented Mar 8, 2024 •

edited

Loading

Flouse commented Mar 8, 2024

		@@ -42,7 +42,7 @@ jobs:

		- name: Install dependencies
		run: pnpm i

feat(rgbpp-ckb): build btc transfer virtual ckb tx #13

feat(rgbpp-ckb): build btc transfer virtual ckb tx #13

Conversation

duanyytop commented Mar 7, 2024 • edited Loading

Changes

Reviewers

Flouse commented Mar 7, 2024 • edited Loading

duanyytop commented Mar 7, 2024

Flouse Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

duanyytop Mar 8, 2024

Choose a reason for hiding this comment

Flouse Mar 7, 2024

Choose a reason for hiding this comment

duanyytop Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Flouse Mar 8, 2024

Choose a reason for hiding this comment

ShookLyngs Mar 7, 2024

Choose a reason for hiding this comment

duanyytop Mar 8, 2024

Choose a reason for hiding this comment

ShookLyngs Mar 8, 2024

Choose a reason for hiding this comment

ShookLyngs commented Mar 8, 2024 • edited Loading

duanyytop commented Mar 8, 2024 • edited Loading

Flouse commented Mar 8, 2024

duanyytop commented Mar 7, 2024 •

edited

Loading

Flouse commented Mar 7, 2024 •

edited

Loading

Flouse Mar 7, 2024 •

edited

Loading

duanyytop Mar 8, 2024 •

edited

Loading

ShookLyngs commented Mar 8, 2024 •

edited

Loading

duanyytop commented Mar 8, 2024 •

edited

Loading