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: molecule codec #88

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

Conversation

ashuralyk
Copy link
Contributor

#Description

migrate whole @ckb-lumos/codec package into ccc and replace with this new codec of ccc for native Spore package, which test cases have passed

Copy link

changeset-bot bot commented Nov 26, 2024

⚠️ No Changeset found

Latest commit: c0374a0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

return result;
},
});
};
Copy link
Member

Choose a reason for hiding this comment

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

import { CodecBaseParseError } from "./error.js";

type BI = Num;
type BIish = NumLike;
Copy link
Member

Choose a reason for hiding this comment

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

BI is different with Num, we should replace these type names.

@@ -0,0 +1,127 @@
import {
Copy link
Member

Choose a reason for hiding this comment

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

Some code in this file is not needed anymore, please remove them.

>;

export type BytesCodec<Unpacked = any, Packable = Unpacked> = Codec<
Uint8Array,
Copy link
Member

Choose a reason for hiding this comment

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

Packable
>;

export type BytesCodec<Unpacked = any, Packable = Unpacked> = Codec<
Copy link
Member

Choose a reason for hiding this comment

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

It seems that the Uint8ArrayCodec should be replaced by BytesCodec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's a little different, BytesCodec accepts BytesLike as parameter for unpack method, in contrast, Uint8ArrayCodec only accepts Uint8Array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can only preserve BytesCodec as the concrete bytes encode/decode, and have every external callers to transfer its BytesLike parameter using ccc.bytesFrom before pack/unpack

const HexUint8 = asHexadecimal(Uint8);
const HexUint32LE = asHexadecimal(Uint32LE);
const HexUint64LE = asHexadecimal(Uint64LE);
const HexUint128LE = asHexadecimal(Uint128LE);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we don't need these, just Uint8 should be enough. We used Uint8 for HashType but HexUint8 for DepType, this doesn't make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hex prefixed types are unpacking to output string, all of these are used in below codes, I'm not sure to delete them.

if (type === "data2") return Uint8.pack(0b0000010_0);
throw new Error(`Invalid hash type: ${type}`);
},
unpack: (buf) => {
Copy link
Member

Choose a reason for hiding this comment

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

if (type === "code") return HexUint8.pack(0);
if (type === "depGroup") return HexUint8.pack(1);
throw new Error(`Invalid dep type: ${type}`);
},
Copy link
Member

Choose a reason for hiding this comment

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

@@ -0,0 +1,8 @@
export { byteArrayOf, byteOf, byteVecOf } from "./helper.js";
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can export everything here?

@@ -9,3 +9,4 @@ export * from "./keystore/index.js";
export * from "./num/index.js";
export * from "./signer/index.js";
export * from "./utils/index.js";
export * as codec from "./codec/index.js";
Copy link
Member

Choose a reason for hiding this comment

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

Personally I think pack unpack Packed Unpacked Packable Unpackable are confusing. It looks like Packed is always Bytes here. Maybe we don't need this much flexibility.

I suggest to use
encode decode DataType instead, and encoder result should always be Bytes.
See https://en.wikipedia.org/wiki/Codec#:~:text=A%20codec%20is%20a%20device,a%20portmanteau%20of%20coder%2Fdecoder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

confusing indeed there, because of it, I'm concerned to alter it, afraid of causing unexpected problems, however, speaking of which, I'd like to have a try fixing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about just changing them to:
Packed => Encoded
Packable => Encodable
Unpacked => Decoded
Unpackable => Decodable

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