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: add management canister methods for interacting with the chunk store #461

Merged
merged 14 commits into from
Apr 8, 2024

Conversation

hpeebles
Copy link
Contributor

@hpeebles hpeebles commented Feb 1, 2024

Description

Adds upload_chunk, stored_chunks, clear_chunk_store and install_chunked_code to the management canister API.

How Has This Been Tested?

I have tested this locally by uploading a wasm comprised of 3 chunks, the I queried stored_chunks which returned the chunks, then I called install_chunked_code which completed successfully, then I called clear_chunk_store, then I queried stored_chunks again and this time it was empty.

Checklist:

  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@hpeebles hpeebles requested a review from a team as a code owner February 1, 2024 11:22
@hpeebles hpeebles marked this pull request as draft February 1, 2024 20:48
@hpeebles hpeebles marked this pull request as ready for review February 25, 2024 20:39
/// The list of chunks that make up the canister wasm
pub chunk_hashes_list: Vec<ChunkHash>,
pub chunk_hashes_list: Vec<Vec<u8>>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be serialized inefficiently when using serde (it'll be fine when using candid, I think?).
We should either use ByteBuf or manually implement serialize/deserialize for this field.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. I noticed that you were using serde_bytes::ByteBuf. My latest commit focused on making the tests pass. And I felt that it's better to keep the types in API as native as possible, instead of forcing the users to import serde_bytes.

I will go through the API data structures to make sure that all blob: Vec<u8> are enhanced with #[serde(with = "serde_bytes")].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah ideally we'd expose Vec<u8> rather than ByteBuf, but when its Vec<Vec<u8>> we can't use the #[serde(with = "serde_bytes")] trick. I think the only way to avoid exposing ByteBuf would be to manually implement serialize and deserialize.

@lwshang
Copy link
Contributor

lwshang commented Feb 29, 2024

@hpeebles

For the inconsistency between the interface-spec and the replica implementation, the conclusion is that the replica will be modified to align with the spec. So we have to wait for that before we can complete this PR.

And since it will be pub chunk_hashes_list: Vec<ChunkHash> in the end, we don't have to implement the custom serde methods now.

Turning this PR back to draft now.

@lwshang lwshang marked this pull request as draft February 29, 2024 17:13
@lwshang lwshang marked this pull request as ready for review April 8, 2024 15:44
@lwshang lwshang merged commit 2d3f76e into dfinity:main Apr 8, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants