Clean up asset_util interface #2099

frederikrothenberger · 2023-12-04T14:40:55Z

This PR cleans up the asset_util crate interface:

hide internals of CertifiedAssets
provide unified interface for both asset certification versions
provide support for canisters that do not certify the sigs subtree
add documentation to the public functions

This PR is step 3 of 4:

move code from assets.rs to the crate
move certification header code from http.rs to the crate
refactor the crate a bit to make the public interface nicer
use the crate in the other canisters

🟡 Some screens were changed

This PR cleans up the asset_util crate interface: * hide internals of `CertifiedAssets` * provide unified interface for both asset certification versions * provide support for canisters that do not certify the `sigs` subtree * add documentation to the public functions This PR is step **3** of 4: 1. move code from `assets.rs` to the crate 2. move certification header code from `http.rs` to the crate 3. refactor the crate a bit to make the public interface nicer 4. use the crate in the other canisters

przydatek

Please see some minor comments/suggestions below.

przydatek · 2023-12-04T15:16:22Z

src/asset_util/src/lib.rs

@@ -28,9 +28,17 @@ pub const IC_CERTIFICATE_EXPRESSION: &str =
 /// certification to be valid.
 #[derive(Debug, Default, Clone)]
 pub struct CertifiedAssets {


nit: maybe rename to CertifiedAssetsStore or something similar, as otherwise it is slightly confusing that we have two structs CertifiedAssets and CertifiedAsset and the former is not a container of the latter (in the definition of the structs).

Well semantically it does. The crate does not give visibility into the internals of CertifiedAssets (and a consumer shouldn't care). But you can retrieve a CertifiedAsset from CertifiedAssets using the certified_asset function. So all is as expected?

Agreed that semantically it is perfectly fine, just reading both struct definitions without visible relationship feels strange. But it is nitpicking, so I'm also fine with keeping as-is.

src/asset_util/src/lib.rs

przydatek · 2023-12-04T15:28:32Z

src/asset_util/src/lib.rs

+                Some(2) => certified_asset
+                    .headers
+                    .extend(self.certificate_headers_v2(url_path, sigs_tree)),
+                // Certification v1 is also the fallback for unknown certificate versions


I wonder whether this fallback is the right thing to do: if at some point v3 is supported, this would silently return v1 even if v3 is requested, right? Maybe it would be better to be more strict about the versions?

Also, this defaults to v1 if caller does not specify any version -- maybe it would be better to default to the newest version supported?

The problem is that originally clients did not specify a version at all. So in order to not break old clients, we have to assume that they only understand v1.
But I'll make the logic more robust, so that anything >=2 uses version 2 and nothing or 1 use version 1. 👍

as discussed, let's rename certificate_version to max_certificate_version, and specify in a comment that it means something like "max. certificate version expected by the caller", and that the actually returned version can be lower.

src/asset_util/src/lib.rs

przydatek

LGTM, thanks!

przydatek · 2023-12-05T13:16:03Z

src/asset_util/src/lib.rs

@@ -28,9 +28,17 @@ pub const IC_CERTIFICATE_EXPRESSION: &str =
 /// certification to be valid.
 #[derive(Debug, Default, Clone)]
 pub struct CertifiedAssets {


Agreed that semantically it is perfectly fine, just reading both struct definitions without visible relationship feels strange. But it is nitpicking, so I'm also fine with keeping as-is.

przydatek · 2023-12-05T14:00:35Z

src/asset_util/src/lib.rs

+                Some(2) => certified_asset
+                    .headers
+                    .extend(self.certificate_headers_v2(url_path, sigs_tree)),
+                // Certification v1 is also the fallback for unknown certificate versions


as discussed, let's rename certificate_version to max_certificate_version, and specify in a comment that it means something like "max. certificate version expected by the caller", and that the actually returned version can be lower.

* Clean up asset_util interface This PR cleans up the asset_util crate interface: * hide internals of `CertifiedAssets` * provide unified interface for both asset certification versions * provide support for canisters that do not certify the `sigs` subtree * add documentation to the public functions This PR is step **3** of 4: 1. move code from `assets.rs` to the crate 2. move certification header code from `http.rs` to the crate 3. refactor the crate a bit to make the public interface nicer 4. use the crate in the other canisters * Add link to certified data and canister signatures * Fall back to v2 for versions higher than 2 * Refactor public interface to only have one function (with options) * Improve certify_assets doc * Make certify_assets a constructor * Clarify the certificate_version parameter

frederikrothenberger requested a review from przydatek December 4, 2023 14:41

przydatek reviewed Dec 4, 2023

View reviewed changes

frederikrothenberger added 5 commits December 5, 2023 10:31

Add link to certified data and canister signatures

8b9efca

Fall back to v2 for versions higher than 2

5b9c94e

Refactor public interface to only have one function (with options)

d0eb706

Improve certify_assets doc

e4e8e29

Make certify_assets a constructor

f18c315

frederikrothenberger requested a review from przydatek December 5, 2023 13:07

przydatek approved these changes Dec 5, 2023

View reviewed changes

frederikrothenberger added 2 commits December 5, 2023 15:03

Clarify the certificate_version parameter

59773b0

Merge branch 'main' into frederik/refactor-interface

08b5424

frederikrothenberger enabled auto-merge December 5, 2023 14:13

frederikrothenberger added this pull request to the merge queue Dec 5, 2023

Merged via the queue into main with commit 442a30e Dec 5, 2023
57 checks passed

frederikrothenberger deleted the frederik/refactor-interface branch December 5, 2023 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up asset_util interface #2099

Clean up asset_util interface #2099

frederikrothenberger commented Dec 4, 2023 •

edited by github-actions bot

Loading

przydatek left a comment

przydatek Dec 4, 2023

frederikrothenberger Dec 5, 2023

przydatek Dec 5, 2023

przydatek Dec 4, 2023

frederikrothenberger Dec 5, 2023

przydatek Dec 5, 2023

przydatek left a comment

przydatek Dec 5, 2023

przydatek Dec 5, 2023

Clean up asset_util interface #2099

Clean up asset_util interface #2099

Conversation

frederikrothenberger commented Dec 4, 2023 • edited by github-actions bot Loading

przydatek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

przydatek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederikrothenberger commented Dec 4, 2023 •

edited by github-actions bot

Loading