Conversation
gregorydemay
force-pushed
the
gdemay/DEFI-2677-canister-blob-storage
branch
from
1eb2964 to
f269c70
Compare
gregorydemay
force-pushed
the
gdemay/DEFI-2677-canister-blob-storage
branch
from
f269c70 to
d9d86b5
Compare
mbjorkqvist
left a comment
mbjorkqvist
left a comment
There was a problem hiding this comment.
Thanks @gregorydemay ! I have some minor comments and questions.
| #[derive(CandidType, Deserialize, Debug, PartialEq, Eq)] | ||
| pub enum InsertError { | ||
| NotAuthorized, | ||
| InvalidHash(String), |
There was a problem hiding this comment.
nit: It's not clear what the String should contain. Digging into the code, it looks like we are populating it with the error message returned if we are unable to parse a (hex) string into a Hash, but we don't check this in tests. Would it make sense to add some rustdoc, or remove the String?
| hex = { workspace = true } | ||
| pocket-ic = { path = "../../../packages/pocket-ic" } | ||
| serde_bytes = { workspace = true } | ||
| sha2 = { workspace = true } |
There was a problem hiding this comment.
nit: hex, serde_bytes, and sha2 are already included in [dependencies], so they could be removed from [dev-dependencies].
| @@ -0,0 +1,32 @@ | |||
| [package] | |||
| name = "blob-store" | |||
There was a problem hiding this comment.
I don't know if we have a naming policy, but I was wondering if this should be something like "ic-blob-store"? I was thinking if it should also have "canister" in the name, but I suppose this could be used to store any type of blobs, e.g., also candid-encoded init or upgrade args.
| pub fn get(&self, hash: &Hash) -> Option<Blob> { | ||
| self.store | ||
| .get(hash) | ||
| .map(|b| Blob::new_unchecked(b, hash.clone())) | ||
| } |
There was a problem hiding this comment.
nit: Could we change this method to take ownership of the hash (i.e., hash: Hash), which would allow us to avoid the clone() in the call to Blob::new_unchecked?
| ) | ||
|
|
||
| rust_canister( | ||
| name = "canister", |
There was a problem hiding this comment.
Would it make sense to name this something other than just "canister" (and keep the name in sync with the package name in the Cargo.toml file)?
|
|
||
| ```bash | ||
| HASH=$(sha256sum ic-icrc1-ledger-u256.wasm.gz | awk '{print $1}') | ||
| icp canister call blob_store insert "(record { hash = \"$HASH\"; data = blob \"$(xxd -p ic-icrc1-ledger-u256.wasm.gz | tr -d '\n')\" })" |
There was a problem hiding this comment.
I couldn't get this command to work - the xxd and tr commands output a hex string, but then icp-cli doesn't seem to interpret it correctly, and the insert fails with a hash mismatch (at least locally on my mac). If I use the "always assist" feature and point icp-cli to the file, the request it ends up sending has data = blob "\1f\8b\08....
Slightly unrelated, but the response from icp-cli doesn't decode the candid into intelligible field names, so I spent some extra time with rainbow tables figuring out what the response was. If there's a way to have the response parsed using the blob_store.did file, it would make sense to have that part of the command here in the README.md. This is what I got:
(
variant {
3_456_837 = variant {
2_789_984_860 = record {
374_295_182 = "dc96f8327298ab7537534d10d8e5b9506dfaa0bd6c2fb05a48ec6cd00ace9172";
1_076_899_448 = "c5108432520195d660d2998379392dd246279afe897bb30111505e3e0590875e";
}
}
},
)
2 participants
Create a simple canister that acts as a blob store addressed by their SHA-256 hashes.
The main use-case will be to deploy such a canister to upload ledger suite wasms so that then can be retrieved by other canisters without additional trust assumptions: the calling canister given a trusted hash (obtained for example through running the reproducible build) can query the blob canister and verify that the obtained blob has the expected hash. In particular, NNS-controlled canisters could depend on such blob store canister without it being under NNS control.