Afrz: ASA Global Freeze #6298
Afrz: ASA Global Freeze #6298
Conversation
ASAs now have a GlobalFrozen flag which prevents all holders from moving the asset if set, it's currently set by a acfg transaction, but this should probably be an afrz transaction instead.
We now specifically use the account being frozen as the check. If it's missing/zero then the freeze is global.
…eeze update "asset_holding_get" and "asset_params_get" opcodes
… has been deleted
…rozen Also updated naming convention
There was a problem hiding this comment.
Various comments about what's here, but also there's something missing. This behaviour needs to be consensus gated. So there's needs to be a flag in config/consensus.go. You would set it to true in vFuture.
You would also make WellFormed check that the freeze target is not 0 unless the config flag is set.
As of today, transactions will always fail if you try to freeze the 0 address, but this will allow it. So unless this gated, nodes will start accepting such transactions as soon as they are upgraded, but before the vote. Disagreement ensues.
cmd/goal/asset.go
Outdated
| fmt.Printf("Global frozen: %v\n", derefBool(asset.Params.GlobalFrozen)) | ||
| fmt.Printf("Last freeze: %v\n", deferUint64(asset.Params.LastFreeze)) |
There was a problem hiding this comment.
I know I proposed the two fields to keep track of global freeze, but I think it can just be one. You really only need LastFreeze if, when unfrozen, you set it back to 0.
There was a problem hiding this comment.
Updated the AssetParams to now use a single uint64 rather than both the bool and uint64. Although on the command line output, how should we present this? The transaction counter value seems a bit irrelevant for most people, should I just replace it with "True"/"False" instead?
There was a problem hiding this comment.
This is why I'm not so sure we should even present it. In a sense, the illusion we're creating is that there's no such thing as "global freeze", it's just that there's one operation that freezes everyone at once.
Of course that's not quite true, but we should be careful about how much we leak this abstraction.
One place I haven't through all the way - when an asset is defaultFreeze=false, but there's currently a global freeze in effect (that is, the last freeze is > 0), what do we do? I would think we do almost nothing, the local freeze state is false, but we allow the existence of the global freeze to override it. When the global freeze is lifted, this account would be unfrozen. We need to make sure we do not populate the local account last-freeze-change at all. It stays zero, because we don't want it to override global freeze just because the opt-in happened afterward.
This create a backward incompatibility. An app could look at the asset params, see that it is defaultFreeze=false, opt a new account in, and that should guarantee that account can transact. But it cannot. Should we be "lying" about defaultFrozen, and return true when there's a global freeze in effect? I lean yes, it's very similar to lying about an account's freeze state.
Ok, so what if asset params say defaultFreeze=true? I think this is simpler. We mark the account locally frozen (the last-freeze-change is irrelevant, I think) regardless of the global freeze state.
There was a problem hiding this comment.
I hadn't considered changing what the DefaultFrozen returns, but I see what you mean.
When it comes to the AVM and preserving backwards compatibility with existing smart contracts, it sounds right to "lie" about fields like DefaultFrozen and AssetFrozen by incorporating the new LastGlobalFrozen state. But I'm not so sure we should do this in places like the API, especially on a field we consider immutable. But now there's a discrepancy between what's evaluated in a smart contract and what the same developer might see on a block explorer.
I'm possibly overthinking it, but will ask around and see how others feel. I think I could be convinced either way.
So to summarise, the following fields should be added/updated in both the AVM and the API to reflect the new LastGlobalFreeze field in the same way, but to not actually share what the LastGlobalFreeze value is:
- AssetParams: DefaultFrozen (update) - True/False or True if LastGlobalFreeze is non-zero.
- AssetParams: Frozen (new) - True if LastGlobalFreeze is non-zero, otherwise False.
- AssetParams: LastGlobalFreeze (new) - Do not expose?
- AssetHolding: IsFrozen (update) - True/False or True if LastGlobalFreeze > LastFreezeChange.
- AssetHolding: LastFreezeChange (new) - Do not expose?
- AssetHolding: LocalFrozen (new) - True/False ignoring LastGlobalFreeze
There was a problem hiding this comment.
Yes, I had not considered that the new lie I'm proposing means that an "immutable" property can be observed to change. That's also a backward incompatibility. I wonder which is worse?
Do we even need to expose Frozen on AssetParams? I don't want to admit to all this LastChanged machinery. Contracts observe that sometimes accounts get frozen all at once, but do they need to know anything else? Maybe it was just a bunch of Freeze operations. It's barely meaningful to expose the global Freeze value on it's own. Even though it's true, many accounts can be unfrozen.
There was a problem hiding this comment.
I think I'm mostly in agreement with your conclusions, but the notion of "whitelisted asset" is not immutable, so I don't know if talking about it helps us here. If there is a FreezeAddress, then with the addition of this feature, every asset can become a "whitelisted asset" by issuing a global freeze. From that point on, nobody can move the asset unless they are unfrozen --- white-listed. There's no inherent reason to assume the global freeze is temporary.
There was a problem hiding this comment.
It is not really the same thing. For sure, if an asset is DefaultFrozen, users are 100% sure the Creator intended to issue a whitelisted asset; with GlobalFrozen, it is not, because it might change. But I get your point, so my argument is weaker.
Changing the DefaultFrozen = False API response depending on the GlobalFrozen, effectively changing an immutable field into a "partially mutable field", seems to me a much worse braking change.
Accounts could have been frozen/unfroze before; it's just that it was done "locally." A Freeze Address might observe the chain and issue a Freeze immediately after an opt-in. This would look like an account opt-in while the asset is globally frozen, but this has never impacted the DefaultFrozen semantic.
There was a problem hiding this comment.
Updating the API to return each asset holdings is-frozen status with respect to the asset params LastGlobalFreeze value would require retireving the asset params for every asset they hold which I don't imagine we want to do for performance reasons.
There was a problem hiding this comment.
with GlobalFrozen, it is not, because it might change
Freezer could apply a global freeze, then manager could remove the freezeAddress from the asset params. That is just as permanent.
A Freeze Address might observe the chain and issue a Freeze immediately after an opt-in. This would look like an account opt-in while the asset is globally frozen, but this has never impacted the DefaultFrozen semantic.
Yes, but the freezer can't "sneak in" to a group. Today, if the defaultFrozen=false, code can observe an account perform an opt-in and know they can transact. I suspect we can live with this change, because there's not really anything you can do with that knowledge, and as soon as your group is over, the freezer could come along and freeze it.
I suppose that case I mentioned above is an exception. Today, if a DEX observes defaultFrozen=false, freezeAddress=zero, and they see an account opt-in, they know that user can never be frozen. But, in the case I described above, that user will be frozen at opt-in (because the freezer left a global freeze in effect), and can never unfreeze.
Should the rule that newly opted-in accounts become frozen if defaultFrozen=false but global freeze is in effect be suspended if the freezeAddress is set to zeroAddress? In a way, we are "pretending" that global freeze is a very watchful freeze account who swoops in immediately. But in this case, the assetParams will say there is no freeze address. So how can it swoop in?
There was a problem hiding this comment.
Freezer could apply a global freeze, then manager could remove the freezeAddress from the asset params. That is just as permanent.
Sure, but this also means whoever was frozen due to the global freeze will stay frozen forever. My point is that this behavior is different from a DefaultFrozen, which is a permanent characteristic of the asset that does not prevent the Freeze Address from keep building a whitelist/blacklist.
Should the rule that newly opted-in accounts become frozen if defaultFrozen=false but global freeze is in effect be suspended if the freezeAddress is set to zeroAddress? In a way, we are "pretending" that global freeze is a very watchful freeze account who swoops in immediately. But in this case, the assetParams will say there is no freeze address. So how can it swoop in?
I'm not sure about this. Maybe I'm missing something obvious, but if the Global Freeze is in place AND the Freeze Address is set to ZeroAddress, this would imply the newly opted-in accounts stay frozen forever, right? If so, are you suggesting that we should be able to perform a "default frozen ex-post"? Something like:
- As the Creator, I create the ASA with
DefaultFrozen=False - As the Freeze Address, I Globally Freeze the ASA
- As the Freeze Address, I Locally Unfreeze some accounts
- As the Manager Address, set the Freeze Address to
ZeroAddress - The ASA is "default frozen" just from now on BUT with a fixed and immutable whitelist.
I guess the ultimate question is: if the ASA is GlobalFrozen, as soon as the Freeze Address is set to ZeroAddress, do we prefer to keep it closed or open for newcomers? I think we should keep it closed (new opt-ins are frozen), and so since it's immutable (FreezeAddress=ZeroAddress), this could be the only edge case in which we might consider the ASA as DefaultFrozen.
Gated via consensus upgrade Simplified AssetParams frozen status to a single uint64 Updated TEAL opcodes to reflect changes Reduced duplicate code in ledger
cmd/goal/account.go
Outdated
| } else if assetParams.Params.LastGlobalFreeze != nil { | ||
| if *assetParams.Params.LastGlobalFreeze > assetHolding.LastFreezeChange { | ||
| frozen = " (globally frozen)" | ||
| } |
There was a problem hiding this comment.
This implies that we're not "lying" to REST APIs like we're doing to AVM code. We don't set assetHolding.IsFrozen when the asset is globally frozen? I think we probably should, I don't see a good reason to treat them differently.
We could add assetHolding.IsLocallyFrozen, just as from AVM.
There was a problem hiding this comment.
The API now returns booleans, abstracting away the uint64 txncounter values.
There was a problem hiding this comment.
Actually I forgot to mention, when retrieving an account it only includes their local frozen state. I would have to lookup all asset params for every asset they hold to change their frozen state with respect to the LastGlobalFreeze.
| Total: params.Total, | ||
| Decimals: uint64(params.Decimals), | ||
| DefaultFrozen: &frozen, | ||
| LastGlobalFreeze: &lastGlobalFreeze, |
There was a problem hiding this comment.
I think this will do it.
| LastGlobalFreeze: &lastGlobalFreeze, | |
| LastGlobalFreeze: omitEmpty(params.LastGlobalFreeze), |
I think DefaultFrozen: omitEmpty(params.DefaultFrozen) would also be an improvement.
There was a problem hiding this comment.
Hmm, if you change DefaultFrozen: omitEmpty(params.DefaultFrozen) then the false is not omitted from the response. I don't understand why. It should be setting the value to nil when false, which I expect to get omitted. Anyway, don't do that.
But, assuming it has the behavior we want, it would still be nice to use omitEmpty on params.LastGlobalFreeze.
We'll need to extend rest-assets-endpoint.sh to have e2e tests of the new stuff either way.
ledger/apply/asset.go
Outdated
| } | ||
|
|
||
| // If FreezeAccount is not set, then this is a global freeze/unfreeze operation. | ||
| if balances.ConsensusParams().AssetGlobalFreeze && cf.FreezeAccount.IsZero() { |
There was a problem hiding this comment.
I think putting the consensus check here is fine, but let's consider if we can/should put it in WellFormed. I think it's currently impossible to freeze the 0 address, because it cannot be opted-in to anything. So maybe we can just outlaw zero freezeaddress up in WellFormed, and not need to repeat the consensus check here.
There was a problem hiding this comment.
I chatted about this offline with other devs. We notice that the asset txns currently don't have any WellFormed checks. To do what I'm describing, we should probably first introduce wellFormed() functions on them (see the pattern in transaction.WellFormed()). For the asset transactions, try to lift stateless checks out of the apply code. The asset ID must be non-zero, the receiver/freezeAddress must be non-zero.
There was a problem hiding this comment.
I'll hopefully get my latest changes pushed tomorrow. I didn't realise how far behind master I was, and I just realised you've done a bunch of work in this exact area. So I'll merge master back into my branch and resolve any conflicts, then add the wellFormed() code for assets.
nullun
force-pushed
the
feature/asa-global-freeze
branch
from
923f12e to
292a68f
Compare
| var lastGlobalFreeze uint64 | ||
| if ca.Params.Frozen != nil { | ||
| // model.Asset doesn't know the LastGlobalFreeze value, so we | ||
| // fake it by setting it to MaxUint64 for basics.AssetParams. | ||
| lastGlobalFreeze = math.MaxUint64 | ||
| } |
There was a problem hiding this comment.
Looking for feedback here. From what I can gather AccountToAccountData() is only used by dryrun/tealdbg and then used in 2 tests, but I'm not sure how much energy is going into maintaining them?
Essentially there's now a difference between what basics.AccountData and model.Account holds.
- We abstract the
basics.AssetHolding.LastFreezeChangeaway inmodel.AssetHoldingafter changing theIsFrozenvalue. - We abstract the
basics.AssetParams.LastGlobalFreezeaway inmodel.AssetParamsafter adding theFrozenboolean value.
Generally this conversion is done in one direction (from basics.AccountData to model.Account), so it didn't seem like a problem. But if using dryrun I'm now lying about the LastGlobalFreeze value, so it would always win regardless of an accounts local Freeze/LastFreezeChange.
| func (ac AssetConfigTxnFields) wellFormed() error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I didn't see anything in the existing code to move here, since you can create an ASA with absolutely nothing defined and it works.
| switch tx.Type { | ||
| case protocol.AssetConfigTx: | ||
| err := tx.AssetConfigTxnFields.wellFormed() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| case protocol.AssetTransferTx: | ||
| err := tx.AssetTransferTxnFields.wellFormed() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| case protocol.AssetFreezeTx: | ||
| err := tx.AssetFreezeTxnFields.wellFormed(proto) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Other than the consensus check (proto.Asset) I didn't see them sharing any similar code. So I should probably elevate these to the outer switch statement.
| // Cannot close by clawback | ||
| if clawback { | ||
| return fmt.Errorf("cannot close asset by clawback") | ||
| } | ||
|
|
There was a problem hiding this comment.
Moved to axfer wellFormed.
Also reenable EnableGlobalFreeze in vFuture, I'd mistakenly removed it during the merge
This PR introduces a mechanism to freeze asset transfers for everyone with a single transaction. Available to new and existing ASAs that have a Freeze Address set.
At present an ASA with a freeze address must send a freeze transaction to each account it wishes to freeze, preventing them from transfering the asset (other than a closeout). This change enables the freeze transactions to be sent with an empty target (zero address) and it'll freeze the asset itself, preventing all holders from sending or recieving the asset. This can also be overriden by sending further asset freeze transactions to accounts to individually unfreeze them. Note however that if an individual account holding is frozen, they cannot transfer their asset even if the global freeze state is unfrozen.
This PR also includes changes to the AVM to keep backwards compatability with the existing
asset_holding_get AssetFrozenopcode, whilst introducing two new fields for more specific behaviour.asset_holding_get AccountFrozenandasset_params_get AssetGlobalFrozen.I've yet to complete and tests for this PR, so setting as draft for now.
goalCLINote: This PR is the result of an Algorand Developer Retreat hackathon. Contributors: Argimiro, Cosimo, Giulio, Ori, and Steve.