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

feat(rgbpp): support for batch transferring of RGBPP XUDT assets #270

Merged
merged 17 commits into from
Aug 12, 2024

Conversation

ShookLyngs
Copy link
Collaborator

@ShookLyngs ShookLyngs commented Aug 6, 2024

Changes

  • Add buildRgbppTransferAllTxs() API to generate one or more BTC/CKB transaction groups for transferring the entire amount of a specific type of RGBPP XUDT asset from one or more BTC addresses to a recipient
  • Add sendRgbppTxGroups() API for sending BTC/CKB transaction groups to the BtcAssetsApi

TODOs

  • Base coding
  • Testing
  • Update docs (README of rgbpp)
  • Update examples? (no place for the example)

@ShookLyngs
Copy link
Collaborator Author

ShookLyngs commented Aug 7, 2024

Mocked a test for the transfer-all-of-an-rgbpp-xudt-asset feature with @Dawn-githup:

  • Transfer all amount of a specific RGBPP XUDT asset XTT, from Alice to Bob
  • Alice holds 41 L2 cells bound to the same L1 output, the total amount is 41 XTT
  • Alice holds another 50 L2 cells, each bound to an individual L1 output, the total amount is 500 XTT

The expected result:

  • Alice transfers 500 XTT to Bob in 2 transactions because each CKB can contain up to 40 RGBPP cells
  • The 41 cells with total value of 41 XTT should be excluded during the transfer process, because too much L2 cells have been bound to a single L1 output, while each CKB transaction can include up to 40 RGBPP cells
  • The L1 fees should be paid with the L1 RGBPP outputs because the fee rate is low

The actual transaction groups generated from the process:

@ShookLyngs ShookLyngs marked this pull request as ready for review August 8, 2024 06:51
.github/workflows/test.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,80 @@
import {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest this file should be moved to rgbpp-sdk/btc and examples can reuse it

Copy link
Collaborator Author

@ShookLyngs ShookLyngs Aug 9, 2024

Choose a reason for hiding this comment

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

Maybe we should, but a concern is we may not handle P2TR well, because it can be complicated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After reviewing the current codebase of the packages/examples, I think this is not a good timing to do this. Examples and tests have different needs for the feature, and it's a big change to refactor them all.

@Flouse
Copy link
Contributor

Flouse commented Aug 9, 2024

  • The L1 fees should be paid with the L1 RGBPP outputs because the fee rate is low

Have you tested the case where the BTC transaction requires additional inputs to pay tx fee?
@ShookLyngs @Dawn-githup

@Dawn-githup
Copy link
Collaborator

  • The L1 fees should be paid with the L1 RGBPP outputs because the fee rate is low

Have you tested the case where the BTC transaction requires additional inputs to pay tx fee? @ShookLyngs @Dawn-githup

When the fee rate is higher, utxo will be collected from the from address to pay the fee.

@Flouse
Copy link
Contributor

Flouse commented Aug 11, 2024

Discussion or TODO

packages/btc/src/utils.ts Show resolved Hide resolved
packages/rgbpp/README.md Outdated Show resolved Hide resolved
@ShookLyngs
Copy link
Collaborator Author

ShookLyngs commented Aug 11, 2024

@duanyytop and I had a few discussions about the PR:

  1. The .env file for the rgbpp lib is redundant, as all tests in the rgbpp lib are mocked. Can it be removed?
  2. Should we keep the rgbpp lib as simple as possible, instead of adding a lot of utilities to it?
  3. Developers may need an example for the transfer-all feature when coding.

My thoughts:

The .env file for the rgbpp lib is redundant, as all tests in the rgbpp lib are mocked. Can it be removed?

The btc/ckb lib contain some methods that are not mockable, meaning that when calling the buildRgbppTransferAllTxs API, actual network requests must be sent. So, while the .env file can be removed, we still need to provide a default environment so the tests can run as expected. I'm not sure if it's worth it.

I think @duanyytop is concerned about redundant test cases, as there are too many "same pieces of code" in the examples and integration tests. If we add more tests to the rgbpp lib alone, that would increase redundancy.

Personally, I believe the unit tests for the rgbpp library are different from the examples and integration tests because the unit tests allow more room for unexpected cases. However, writing tests for the buildRgbppTransferAllTxs API is challenging and requires a lot of preparation. Maybe @Dawn-github can write integration tests for the feature, and then we can remove the mocked tests from the rgbpp lib, making the .env file unnecessary.

Should we keep the rgbpp lib as simple as possible, instead of adding a lot of utilities to it?

Agreed, so I moved most of the utilities from the rgbpp lib to the btc/ckb libs at c492cf3.

Developers may need an example for the transfer-all feature when writing.

Yes, but it would be hard to write a set of examples just to prepare the assets. If we provide only one example for users who already have many assets, it would be much easier. We may add such example in examples/rgbpp/xudt/btc-transfer-all/1-btc-transfer-all.ts.

Flouse
Flouse previously approved these changes Aug 12, 2024
@Dawn-githup
Copy link
Collaborator

Regarding the transfer-all example, we are currently facing an issue: if we use this file to create a BTC account and sign transactions, with createBtcAccount it won’t support mixed-type addresses when using .env.

  • For example, if we write it as "A wants to transfer all of their xudt1 to B," then the fee and change can go to A, and there's no need for multiple accounts. Would such simple support be sufficient for the example?

@Flouse @ShookLyngs

@ShookLyngs
Copy link
Collaborator Author

ShookLyngs commented Aug 12, 2024

Regarding the transfer-all example, we are currently facing an issue: if we use this file to create a BTC account and sign transactions, with createBtcAccount it won’t support mixed-type addresses when using .env.

I think it depends on how complicated the examples need to be. If we're writing a simple example that transfers all XUDT1 from A to B, then a single account is enough. However, if we're writing an example that transfers all XUDT1 from [A, B] to C, then we'll need at least 2 accounts for signing.

It's best to draft a TODO list of the examples first. cc @Flouse @Dawn-githup

@Flouse Flouse enabled auto-merge August 12, 2024 14:40
@Flouse Flouse added this pull request to the merge queue Aug 12, 2024
Merged via the queue into develop with commit 215dd98 Aug 12, 2024
3 checks passed
@Flouse Flouse deleted the feat/l1-transfer-all branch August 12, 2024 16:36
@Dawn-githup
Copy link
Collaborator

It's best to draft a TODO list of the examples first. cc @Flouse @Dawn-githup

  • This is a simple example using a single account transfer-all

10f0457

@Flouse
Copy link
Contributor

Flouse commented Aug 13, 2024

  • This is a simple example using a single account transfer-all

10f0457

ok, please add an introduction to https://github.com/ckb-cell/rgbpp-sdk/blob/develop/examples/rgbpp/README.md to explain the use case of this example later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants