fix(standards): declare faucet callback slots in TokenPolicyManager schema - #3648
Conversation
mmagician
left a comment
mmagician
left a comment
There was a problem hiding this comment.
Overall LGTM, but this simple change should not be accompanied by that much code comments IMO
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a couple of comments inline.
| WordSchema::new_simple_with_default( | ||
| SchemaType::native_word(), | ||
| Self::invoke_receive_policy_root().as_word(), | ||
| ), |
There was a problem hiding this comment.
Not for this PR, but we should introduce a new SchemaType which specifically indicates that we are storing a root of a procedure in this storage slot. Let's create an issue for this.
| if self.has_transfer_policy() { | ||
| let callback_slots = AssetCallbacks::new() | ||
| .on_before_asset_added_to_account(Self::invoke_receive_policy_root().as_word()) | ||
| .on_before_asset_added_to_note(Self::invoke_send_policy_root().as_word()) | ||
| .into_storage_slots(); | ||
| slots.extend(callback_slots); | ||
| } |
There was a problem hiding this comment.
IIUC, this would cause a divergence between the actual storage of an account the storage schema. Specifically, when has_transfer_policy() == false, the storage would not contain these 2 slots, while the storage schema will (since we add it unconditionally). So, I don't know if we've actually fixed the issue:
- Before this PR, storage and schema would diverge when
has_transfer_policy() == true. - After this PR, storage and schema would diverge when
has_transfer_policy() == false.
I think there are two potential ways to fix it:
- Add these two storage slots unconditionally (same as we do with the schema). The downside is that this would waste 2 slots for components that don't need callbacks.
- Change how we build component metadata - instead of making it struct-based, we can make it instance based (i.e.,
component_metadata()would take&selfas a parameter).
The second option a more "correct" fix - but I haven't thought through of all the downstream impacts of this (i.e., don't know how difficult it is to implement).
There was a problem hiding this comment.
Not directly related to this PR, but I think this problem is also present in #3587, since we can't express conditionals in the toml structure. So we'd probably have to define all slots that could be present in the toml and then selectively remove them some of them from Rust code if they are not present for a particular configuration (i.e. in the component_metadata accessor). For example, Authority wouldn't have the RBAC procedure roots slot if it was configured as OwnerControlled.
Not very clean, but I can't think of a better solution. cc @TomasArrachea
There was a problem hiding this comment.
I'm not sure this comment was addressed - we still have divergence between storage and storage schema (now this happens when has_transfer_policy() == false) - or am I missing something?
Created #3772 for this.
…lots # Conflicts: # crates/miden-standards/src/account/policies/manager.rs
4 participants
Closes: #3647