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!: split candid crate #471

Merged
merged 25 commits into from
Oct 23, 2023
Merged

feat!: split candid crate #471

merged 25 commits into from
Oct 23, 2023

Conversation

lwshang
Copy link
Contributor

@lwshang lwshang commented Oct 4, 2023

Currently, the candid crate mainly consists of two parts:

  • Candid data encoding/decoding
  • Parser and derived functionality (bindgen)

The second part evolves fast which forces us to bump major version frequently.
Such version bumps propagated to the downstream projects (agent-rs, cdk-rs, sdk, ic monorepo, etc).

By splitting out candid_parser, we expect to have a stable candid crate.

Also, I move Principal into a separate ic_principal crate. Therefore, we can provide a lightweight dependency for those who only need Principal.

Notable changes

  • Some private or crate public methods are now public since they are called cross-crate
  • Add parse_idl_args() and parse_idl_value() because in candid_parser we cannot implement FromStr for IDLArgs and IDLValue which are defined in candid.
  • TypeEnv is defined in candid, so we can't implement type method ast_to_type in candid_parser. Instead I turned it into a function which takes TypeEnv as the first argument. Similarly, size_helper() and size() are changed.

Checklist

  • Overhaul the Cargo.toml files
  • Changelog

@dfinity dfinity deleted a comment from github-actions bot Oct 4, 2023
@dfinity dfinity deleted a comment from github-actions bot Oct 4, 2023
@dfinity dfinity deleted a comment from github-actions bot Oct 4, 2023
Copy link
Contributor

@chenyan-dfinity chenyan-dfinity left a comment

Choose a reason for hiding this comment

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

Mostly LG. Can you also update the changelog?

rust/candid_parser/src/error.rs Outdated Show resolved Hide resolved
rust/candid/Cargo.toml Show resolved Hide resolved
rust/candid/src/bindings/mod.rs Outdated Show resolved Hide resolved
@dfinity dfinity deleted a comment from github-actions bot Oct 12, 2023
@dfinity dfinity deleted a comment from github-actions bot Oct 12, 2023
@dfinity dfinity deleted a comment from github-actions bot Oct 12, 2023
@dfinity dfinity deleted a comment from github-actions bot Oct 12, 2023
@lwshang lwshang changed the title feat!: split out candid_parser crate feat!: split out candid crate Oct 12, 2023
@dfinity dfinity deleted a comment from github-actions bot Oct 12, 2023
@github-actions
Copy link

Benchmark for c99ab8d

Click to view benchmark
Test Base PR %
Blob/&str 245.4±70.88µs 247.0±70.86µs +0.65%
Blob/ByteBuf 158.0±46.41µs 155.2±47.27µs -1.77%
Blob/Bytes 112.7±43.36µs 113.8±44.02µs +0.98%
Blob/String 319.3±150.88µs 322.0±152.73µs +0.85%
Collections/vec (text, nat) 70.4±2.07ms 70.2±1.44ms -0.28%
Collections/vec int 32.2±0.09ms 32.2±0.09ms 0.00%
Collections/vec int64 19.2±0.31ms 18.7±0.30ms -2.60%
Collections/vec nat8 14.3±0.03ms 14.7±0.14ms +2.80%
option list/1024 1378.9±1.80µs 1381.9±2.69µs +0.22%
profiles/1024 2.7±0.00ms 2.7±0.02ms 0.00%
variant list/1024 1121.5±2.52µs 1119.2±4.94µs -0.21%

@lwshang lwshang changed the title feat!: split out candid crate feat!: split candid crate Oct 13, 2023
@dfinity dfinity deleted a comment from github-actions bot Oct 13, 2023
@dfinity dfinity deleted a comment from github-actions bot Oct 13, 2023
@lwshang lwshang marked this pull request as ready for review October 13, 2023 00:03
Copy link
Contributor

@chenyan-dfinity chenyan-dfinity left a comment

Choose a reason for hiding this comment

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

Let's merge this to a beta branch. I have a few other breaking changes to make after this. Let's aim for an official release early November.

"rust/candid_derive",
"rust/ic_principal",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you recall why we put Principal back to candid a while ago? Also do we want to give it a more general name, e.g. ic_types, to accommodate more types in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We used to have the ic-types crate which contained principal and hash_tree modules.
The modules were moved to candid and agent-rs repo respectively. And ic-types is deprecated.

IMO, it's better to have ic_principal crate which contains only Principal. It will be a stable crate on the top of the dependency tree. We won't have to bump its version because of changes in other parts.

@lwshang
Copy link
Contributor Author

lwshang commented Oct 13, 2023

I just pushed the beta branch which is identical to this lwshang/split branch.

@chenyan-dfinity chenyan-dfinity changed the base branch from master to beta October 23, 2023 22:18
@github-actions
Copy link

Benchmark for 94c03cc

Click to view benchmark
Test Base PR %
Blob/&str 262.0±84.31µs 263.1±88.02µs +0.42%
Blob/ByteBuf 232.5±76.58µs 223.1±55.06µs -4.04%
Blob/Bytes 151.0±58.76µs 160.8±57.55µs +6.49%
Blob/String 362.3±158.23µs 367.2±167.97µs +1.35%
Collections/vec (text, nat) 82.3±3.56ms 82.4±4.10ms +0.12%
Collections/vec int 36.2±1.65ms 37.3±2.24ms +3.04%
Collections/vec int64 20.8±0.98ms 21.4±1.30ms +2.88%
Collections/vec nat8 16.2±0.61ms 15.7±0.51ms -3.09%
option list/1024 1624.1±86.29µs 1639.4±76.59µs +0.94%
profiles/1024 3.1±0.15ms 3.1±0.13ms 0.00%
variant list/1024 1280.4±61.30µs 1324.7±110.38µs +3.46%

@chenyan-dfinity chenyan-dfinity merged commit aef7dcd into beta Oct 23, 2023
4 checks passed
@chenyan-dfinity chenyan-dfinity deleted the lwshang/split branch October 23, 2023 22:33
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