[libra framework] Separate freezing out into separate module, add freezing to VASPs #4937
Conversation
|
instead of removing is_frozen attribute, can you add logic to implement that in the view ? Otherwise we would need an new API for it again. |
|
Shouldn't be an issue to implement in the view. I'll update. |
There was a problem hiding this comment.
Looks great to me from the Move perspective! The only thing I'm wondering is whether moving the freeze/unfreeze events to a new module is going to create churn for AOS (@moeziniaf?)
| let initiator_address = Signer::address_of(account); | ||
| // The libra root account and TC cannot be frozen | ||
| assert(frozen_address != CoreAddresses::LIBRA_ROOT_ADDRESS(), ECANNOT_FREEZE_LIBRA_ROOT); | ||
| assert(frozen_address != CoreAddresses::TREASURY_COMPLIANCE_ADDRESS(), ECANNOT_FREEZE_TC); |
There was a problem hiding this comment.
Do we want to check an address or a role here (thinking of a future where there are multiple TC addresses...). Same question for the previous line, though perhaps less important given that there is supposed to be only one LR account...
There was a problem hiding this comment.
Good question! I would like a role check here, however all of the role check functions take a &signer whereas we only have an address at this point. So we could update the has_role_* functions to take an address argument instead of a &signer, but doing that does make those functions a bit more "loose" in a sense (since you no longer have the direct connection to authority over the account), but I don't think it would be too bad.
| @@ -260,6 +261,15 @@ module VASP { | |||
| } | |||
| } | |||
|
|
|||
| /// A VASP account is frozen if itself is frozen, or if its parent account is frozen. | |||
| public fun is_frozen(addr: address): bool | |||
| @@ -970,7 +887,8 @@ module LibraAccount { | |||
| // Verify that the transaction sender's account exists | |||
| assert(exists_at(transaction_sender), EPROLOGUE_ACCOUNT_DNE); | |||
|
|
|||
| assert(!account_is_frozen(transaction_sender), EPROLOGUE_ACCOUNT_FROZEN); | |||
| assert(!AccountFreezing::account_is_frozen(transaction_sender), EPROLOGUE_ACCOUNT_FROZEN); | |||
| assert(!VASP::is_frozen(transaction_sender), EPROLOGUE_ACCOUNT_FROZEN); | |||
There was a problem hiding this comment.
Might be worth adding a comment explaining why there's a separate check for VASPs.
Thanks! I believe we're just tailing the chain for Preburn events right now so doesn't seem to be an issue from our end |
|
☔ The latest upstream changes (presumably e3710c1) made this pull request unmergeable. Please resolve the merge conflicts. |
| AccountFreezing::account_is_frozen(parent_address(addr)) || | ||
| AccountFreezing::account_is_frozen(addr) |
|
@bors-libra r=sblackshear |
|
📌 Commit 636f59d has been approved by |
|
Cluster Test Result Repro cmd: |
|
☀️ Test successful - checks-actions_land_blocking_test, checks-circle_commit_workflow |
This pulls account freezing out into its own module. The idea being that this module provides the "atomic semantics" of what it means for an account to be frozen. Other modules (e.g., VASP) can then use these semantics to provide a more semantically rich version of "freezing" e.g., if the parent account is frozen all of the child accounts are frozen, but if the child account is frozen then only that account is viewed as frozen.
Additionally, the libra root account, and TC account cannot be frozen. This is ensured in the
AccountFreezingmoduleWon't land until all spec changes have passed.