Skip to content
This repository has been archived by the owner on Nov 28, 2022. It is now read-only.

refactor: Principal overhaul #17

Merged
merged 25 commits into from Jun 15, 2022
Merged

refactor: Principal overhaul #17

merged 25 commits into from Jun 15, 2022

Conversation

lwshang
Copy link
Contributor

@lwshang lwshang commented Jun 12, 2022

This refactor introduced breaking changes, so bumped up version to 0.4.0-beta.0.

Breaking changes

  • Remove PrincipalInner
    • Principal directly holds len and bytes fields
  • PrincipalError enum has different set of variants reflecting changes in from_text logic.
  • from_text accepts input containing uppercase letters which results in Err before.
  • from_text verifies CRC32 check sequence

Implementation details

  • Mark Principal::from_slice as deprecated
    • It panics if input longer than 29 bytes
    • recommend to use try_from_slice
  • Move all tests to root/tests folder so that we only test public visible behavior
  • Use data-encoding instead of base32
    • base32 wrongly accepts invalid base32 string which makes error handling in from_text not clear
    • data-encoding is better maintained and has more dependents and downloads
  • Use serde_test
    • serde_json and serde_cbor are kept in dev-dependencies because of doc-tests
  • Use impl::impl! macro to verify the necessary traits are implemented

Discussion about data layout

struct Principal(PrincipleInner);
#[repr(packed)]
struct PrincipleInner {
  len: u8,
  bytes: [u8; 29],
}

After removing the layer of PrincipalInner, it becomes:

#[repr(packed)]
struct Principal {
  len: u8,
  bytes: [u8; 29],
}

I'm not sure whether we should keep the repr(packed) here. The Rust Nomicon says:

repr(packed) forces Rust to strip any padding, and only align the type to a byte. This may improve the memory footprint, but will likely have other negative side-effects.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8b19c6d46a7600f575cc936c9dd746ca
With or without repr(packed), the type always has size of 30.

@lwshang lwshang requested a review from a team as a code owner June 12, 2022 01:25
@lwshang
Copy link
Contributor Author

lwshang commented Jun 15, 2022

I created a repo to illustrate that with or without #[repr(packed)], Principal always has size of 30 on wasm32 platform.
So it should be safe to remove the annotation.

I also made a draft PR to candid which makes sure that candid won't be broken.

@lwshang lwshang requested review from a team and ericswanson-dfinity June 15, 2022 19:55
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@lwshang lwshang merged commit ef097d0 into main Jun 15, 2022
@lwshang lwshang deleted the lwshang/principal_overhaul branch June 15, 2022 20:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants