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-sdk-service): support generate_rgbpp_transfer_all_txs API #278

Merged
merged 4 commits into from
Aug 15, 2024

Conversation

ShookLyngs
Copy link
Collaborator

@ShookLyngs ShookLyngs commented Aug 13, 2024

Changes

  • Support generate_rgbpp_transfer_all_txs in the rgbpp-sdk-service for batch transferring of RGB++ xUDT assets, previously implemented at feat(rgbpp): support for batch transferring of RGBPP XUDT assets #270
  • Refactor toSnakeCase() and toCamelCase() to provide clearer return types
  • Fix "module": "NodeNext" related import path issues
    • The src/* path shortcut does not work
    • Relative imports must end with .js
  • Add rgbpp-sdk-service tests to the test.yml workflow

Found issues

Case transformation issue

The toSnakeCase() util transforms all object keys to snake_case, e.g. snakeCase to snake_case. But when transforming the TransactionGroupSummary object, an issue was found that if the object key is a hex string, it will also be transformed as snake_case, e.g. 0xe1e to 0_xe_1e. This is unexpected.

In the current implementation of the generate_rgbpp_transfer_all_txs API, I wrapped the result with an util ensureSafeJson(), which detects if the object key starts with 0_x, then all underscore marks in the key will be removed, reverting it to a regular hex string, e.g. 0_xe_1e to 0xe1e.

// XXX: Remove underscores from hex strings due to toSnakeCase() transformation issue
if (key.startsWith('0_x')) {
  key = key.replaceAll('_', '');
}

BigInt to String issue

The requests in the rgbpp-sdk-service returns JSON as response. An JSON-related issue was found that it doesn't serialize BIgInt values. When an object that contains such type of value is passed to JSON.stringify(), the function throws an error:

TypeError: Do not know how to serialize a BigInt

Currently, I just transform all BIgInt values to hex strings:

// XXX: Convert BigInt to hex string for JSON.stringify() compatibility
if (typeof value === 'bigint') {
  obj[key] = `0x${value.toString(16)}`;
} else {
  obj[key] = value;
}

@duanyytop
Copy link
Collaborator

an issue was found that if the object key is a hex string, it will also be transformed as snake_case, e.g. 0xe1e to 0_xe_1e. This is unexpected.

Why does a key with 0x-hex-string appear?

@duanyytop
Copy link
Collaborator

duanyytop commented Aug 14, 2024

Relative imports must end with .js

May I ask why the .js suffix is needed? It worked fine in my local computer without type: module and .js suffix.

@@ -0,0 +1,28 @@
import isPlainObject from 'lodash/isPlainObject.js';

export function ensureSafeJson<Input extends object, Output = Input>(json: Input): Output {
Copy link
Collaborator

@duanyytop duanyytop Aug 14, 2024

Choose a reason for hiding this comment

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

We should avoid BigInt data in the parameter instead of converting BigInt to hex-string in the JSON stringify function.

It is recommended not to introduce unnecessary complexity, as it may lead to potential bugs.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/search?q=repo%3Ackb-cell%2Frgbpp-sdk%20%3A%20bigint%3B&type=code shows that bigint has been used in packages/ckb/src/types.
So, maybe this ensureSafeJson function is necessary in the current stage?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. However, not all types will be converted to JSON strings. If we add the ensureSafeJson function, we should add tests to ensure it works fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I can add some tests for it. Bold question: Is it possible to refactor all usages of the native bigint to use BI and hex strings instead? Would this be considered a breaking change, or is it acceptable to refactor it partially?

Copy link
Collaborator

@duanyytop duanyytop Aug 14, 2024

Choose a reason for hiding this comment

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

Only affects JSON stringify for BigInt.

So I don't think it is necessary to refactor.

Copy link
Collaborator Author

@ShookLyngs ShookLyngs Aug 14, 2024

Choose a reason for hiding this comment

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

Updated at 27bb01e; added tests for the util functions, and the test.yml workflow now includes the tests for rgbpp-sdk-service. Another change is that I replaced jest with vitest because jest was buggy when bundling/compiling the source code in ESM mode.

export type SnakeCased<T> = SnakeCasedPropertiesDeep<T>;
export type CamelCased<T> = CamelCasedPropertiesDeep<T>;

export const toSnakeCase = <T>(obj: T): SnakeCased<T> | null => {
Copy link
Collaborator

@duanyytop duanyytop Aug 14, 2024

Choose a reason for hiding this comment

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

Maybe we can use this package https://www.npmjs.com/package/camelcase-keys to convert data to snake case with deep mode and it is more simple.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think you mean: https://github.com/bendrucker/snakecase-keys

This is actually a good idea, not because the type the package provides, but because it provides a exclude option for excluding strings from being transformed. If everything goes well, this package should help resolve the 0xe1e to 0_xe_1e issue in another way. I'll give it a try.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@ShookLyngs ShookLyngs Aug 14, 2024

Choose a reason for hiding this comment

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

Updated at 27bb01e; both toSnakeCase() and toCamelCase() have been reimplemented using snakecase-keys and camelcase-keys, for the exclude feature they provide.

Flouse
Flouse previously approved these changes Aug 14, 2024
Copy link
Contributor

@Flouse Flouse left a comment

Choose a reason for hiding this comment

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

LGTM

duanyytop
duanyytop previously approved these changes Aug 14, 2024
@ShookLyngs ShookLyngs dismissed stale reviews from duanyytop and Flouse via 27bb01e August 14, 2024 14:59
@ShookLyngs ShookLyngs added this pull request to the merge queue Aug 15, 2024
Merged via the queue into develop with commit 9daf4ae Aug 15, 2024
3 checks passed
@ShookLyngs ShookLyngs deleted the feat/app-transfer-all-api branch August 15, 2024 01:44
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.

3 participants