feat(icrc-ledger-types): add MetadataKey type #8216
Conversation
… of github.com:dfinity/ic into mducroux/DEFI-1010-add-metadata-keys-icrc-ledger-types
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
mducroux
dismissed
github-actions[bot]’s stale review
No canister behavior changes.
mbjorkqvist
left a comment
mbjorkqvist
left a comment
There was a problem hiding this comment.
Thanks @mducroux, this is a much more comprehensive implementation than what I was expecting! I have some comments/questions regarding the checks and panics.
| /// Parses a metadata key from a string in the format `<namespace>:<key>`. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if the string does not follow the required format. | ||
| pub fn parse(s: &str) -> Result<Self, MetadataKeyError> { | ||
| let colon_pos = s.find(':').ok_or(MetadataKeyError::MissingColon)?; | ||
| let namespace = &s[..colon_pos]; | ||
| let key = &s[colon_pos + 1..]; | ||
|
|
||
| if namespace.is_empty() { | ||
| return Err(MetadataKeyError::EmptyNamespace); | ||
| } | ||
| if key.is_empty() { | ||
| return Err(MetadataKeyError::EmptyKey); | ||
| } | ||
| Ok(Self(s.to_string())) | ||
| } |
There was a problem hiding this comment.
I wonder if we need a similar check also in the init and post_upgrade of the ledger. Currently, no check on the metadata key is performed there, since the MetadataKey instance is created by deserializing the Candid init/upgrade arguments, This means that if a user e.g., constructs the init/upgrade arguments manually on the command line when installing/upgrading a canister using dfx, then they may still end up setting some metadata where the key doesn't follow the <namespace>:<key> format. In an integration test using StateMachine, you can get the same behavior by creating the metadata key to be included in the init/upgrade args using something like the following:
let invalid_string = "invalid_key_without_colon";
let encoded_bytes = Encode!(&invalid_string).unwrap();
let invalid_metadata_key: MetadataKey = Decode!(&encoded_bytes, MetadataKey).unwrap();
I'm not sure if we'd want to make existing ledgers un-upgradable or forcefully discard currently set metadata that doesn't conform to the <namespace>:<key> format. The fact that you cannot add new metadata, but rather only set all metadata, in an upgrade, presents a bit of a problem. If someone has set some metadata with a key without a namespace, and later wants to add/remove/change any (other) metadata, they need to specify all the metadata fields, including ones that won't be changing. Therefore, if we add a check forbidding invalid metadata keys, ledger maintainers would need to choose between keeping the metadata with the invalid keys, or not changing the other ones. One option could be to only apply a strict check in case the existing metadata has no invalid keys. WDYT?
| /// Create a `(MetadataKey, MetadataValue)` tuple for use in metadata maps. | ||
| /// | ||
| /// The key must be a valid metadata key in the format `<namespace>:<key>`. | ||
| /// This is typically used with the predefined constants like `MetadataKey::ICRC1_NAME`. | ||
| /// | ||
| /// # Panics | ||
| /// | ||
| /// Panics if the key is not a valid metadata key format. | ||
| pub fn entry(key: &str, val: impl Into<MetadataValue>) -> (MetadataKey, Self) { | ||
| let metadata_key = | ||
| MetadataKey::parse(key).unwrap_or_else(|e| panic!("invalid metadata key '{key}': {e}")); | ||
| (metadata_key, val.into()) |
There was a problem hiding this comment.
I'm not sure if panicking on bad input here is the best option. I think this is currently only used in tests in our code, but if someone were to e.g., use this in a canister or some other production code for setting metadata in another canister, returning a Result may be more manageable?
| /// Returns the namespace part of the key. | ||
| pub fn namespace(&self) -> &str { | ||
| self.0 | ||
| .find(':') | ||
| .map(|pos| &self.0[..pos]) | ||
| .expect("BUG: MetadataKey should always contain a colon") | ||
| } | ||
|
|
||
| /// Returns the key part (after the namespace). | ||
| pub fn key(&self) -> &str { | ||
| self.0 | ||
| .find(':') | ||
| .map(|pos| &self.0[pos + 1..]) | ||
| .expect("BUG: MetadataKey should always contain a colon") | ||
| } |
There was a problem hiding this comment.
As mentioned in the comment to parse above, we may have deployments where ledgers have metadata keys that do not conform to the <namespace>:<key> format. These namespace() and key() methods aren't currently used in any of our production code, so for the moment we should be fine, but in other production scenarios panicking here may be sub-optimal. I'm not sure what the best alternative would be though - should we for a MetadataKey without a colon return "", None, or an Error for the namespace?
3 participants
DEFI-1010: Add the type MetadataKey to encode ICRC metadata keys as defined here. Replace the usage of String with MetadataKey were applicable.