Skip to content

feat(core): CKB_TX_MESSAGE_ALL #202

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

XuJiandong
Copy link

described in: nervosnetwork/rfcs#446

Copy link

changeset-bot bot commented May 7, 2025

🦋 Changeset detected

Latest commit: 396dbb4

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 Patch
@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

@XuJiandong
Copy link
Author

integration tests: xxuejie/ckb-tx-message-all-test-vector-utils#2

@XuJiandong XuJiandong marked this pull request as ready for review May 8, 2025 01:52
Copy link
Member

@Hanssen0 Hanssen0 left a comment

Choose a reason for hiding this comment

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

This PR provides a good start for CCC to support MESSAGE_ALL signing in the future. However, some API designs need to be discussed more before merging.

Copy link
Member

@Hanssen0 Hanssen0 left a comment

Choose a reason for hiding this comment

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

I noticed that the logic of hashBytes[Opt]ToHasher can actually be replaced by mol.Bytes[Opt]. I suggest using mol.Bytes[Opt].encode directly to save some code here.

By the way, please add a changeset using pnpm changeset in the project directory.

Anyway, I think this code is good enough to be merged. Thank you so much for the contribution. Should we merge it directly, or wait until the proposal is confirmed?

@XuJiandong
Copy link
Author

XuJiandong commented May 13, 2025

I noticed that the logic of hashBytes[Opt]ToHasher can actually be replaced by mol.Bytes[Opt]. I suggest using mol.Bytes[Opt].encode directly to save some code here.

According to the RFC:

The length of the witness field, packed in little-endian encoded unsigned 32-bit integer.
The full witness field, or the full content returned by Load Witness syscall, given the correct index and source

The hashBytesToHasher function follows the RFC mentioned above(Although It uses the same encoding as Molecule bytes, it doesn't mention molecule encoding). I believe this version is much cleaner and easier to understand.

The hashBytesOptToHasher function adds an extra header, based on BytesOpt encoding. When BytesOpt is none, it results in zero bytes, so we include an extra header to differentiate it. Here, I've added a molecule encode, according to the RFC.

Copy link
Member

@Hanssen0 Hanssen0 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

2 participants