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

refactor: split ckb-gen-types from ckb-types #4073

Merged
merged 24 commits into from
Aug 17, 2023

Conversation

EthanYuan
Copy link
Collaborator

@EthanYuan EthanYuan commented Jul 20, 2023

What problem does this PR solve?

Split out Molecule generated types while providing no-std support to ckb-std, replacing crates ckb-standalone-types.

What is changed and how it works?

  • Separated generated types from ckb-types, named as ckb-gen-types.
  • ckb-types re-exports ckb-gen-types to ensure compatibility with other crates that reference it.
  • ckb-gen-types comes with no-std support and can fully replace ckb-standalone-types, which ckb-std relies on.
  • The basic principles of extracting conversions and extensions in ckb-gen-types are as follows:
    • If the conversion or extension involves std types, it will be kept in ckb-gen-types and conditionally compiled with the "std" feature.
    • If the conversion or extension involves types from third-party crates, it will also remain in ckb-gen-types.
    • If the conversion or extension involves types from ckb-types::core, it will be kept in ckb-types, and a custom trait will be placed in the prelude to address orphan rules.
  • ckb-gen-types adds feature controls to several sub-modules in the extension, following the approach used in ckb-standalone-types to be compatible with ckb-std:
    • calc-hash: no-std
    • check-data: no-std
    • serialized-size: no-std
    • std: default, enables all in the std environment.
  • A feature, ckb-contract, has been added to ckb-hash to support compatibility with ckb-std and allow specifying the use of blake2b-ref, following the approach used in ckb-standalone-types.
  • Made some trade-offs in ckb-gen-types (perhaps a better approach could be considered):
    • To maintain compatibility with ckb-std, core::ScriptTypeHash have been kept in ckb-gen-types.

What's Changed:

Related changes

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
    • After replacing ckb-standalone-types with ckb-gen-types, ckb-std passes the tests successfully.
  • No code ci-runs-only: [ quick_checks,linters ]

Side effects

  • Performance regression
  • Breaking backward compatibility

Release note

Title Only: Include only the PR title in the release note.

@EthanYuan EthanYuan force-pushed the refactor-ckb-types branch 7 times, most recently from d21cafe to 335a9c6 Compare July 25, 2023 09:38
@EthanYuan EthanYuan marked this pull request as ready for review July 25, 2023 11:49
@EthanYuan EthanYuan requested a review from a team as a code owner July 25, 2023 11:49
@EthanYuan EthanYuan requested review from doitian and zhangsoledad and removed request for a team July 25, 2023 11:49
@EthanYuan EthanYuan force-pushed the refactor-ckb-types branch from 3fc453b to 5be965e Compare July 27, 2023 06:52
@eval-exec eval-exec self-requested a review August 4, 2023 00:40
@EthanYuan EthanYuan force-pushed the refactor-ckb-types branch 2 times, most recently from c149793 to 2022674 Compare August 16, 2023 17:48
@zhangsoledad zhangsoledad added this pull request to the merge queue Aug 17, 2023
Merged via the queue into nervosnetwork:develop with commit 56e6c26 Aug 17, 2023
@EthanYuan EthanYuan deleted the refactor-ckb-types branch August 21, 2023 06:53
@EthanYuan EthanYuan mentioned this pull request Sep 1, 2023
doitian added a commit that referenced this pull request Sep 7, 2023
/// and then it relies on the high 7 bits to indicate
/// that the data actually corresponds to the version.
#[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)]
pub enum ScriptHashType {
Copy link
Collaborator

@eval-exec eval-exec Sep 12, 2023

Choose a reason for hiding this comment

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

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

Successfully merging this pull request may close these issues.

4 participants