Txn: move/add asset txn validation into their own wellFormed methods

[nullun]

Whilst going through the specs I've been doubling checking what's there and how go-algorand is doing it. For asset transactions it felt like there should be more than we currently had. So I've created this PR to see if it's worth relocating the existing acfg checks and introducing some new axfer/afrz wellFormed checks which I feel make sense.

acfg - moved:

• AssetName must not be longer than MaxAssetNameBytes. This was moved from data/transactions/transaction.go:Transaction.WellFormed().
• UnitName must not be longer than MaxAssetUnitNameBytes. This was moved from data/transactions/transaction.go:Transaction.WellFormed().
• URL must not be longer than MaxAssetURLBytes. This was moved from data/transactions/transaction.go:Transaction.WellFormed().
• Decimals must not be greater than MaxAssetDecimals. This was moved from data/transactions/transaction.go:Transaction.WellFormed().

axfer - new:

• XferAsset must not be zero (missing). This was previously caught by ledger/apply/asset.go as "asset 0 does not exist or has been deleted".
• AssetSender and AssetCloseTo cannot both be set. This was previously caught by ledger/apply/asset.go as "cannot close asset by clawback".

afrz - new:

• FreezeAsset must not be zero (missing). This was previously caught by ledger/apply/asset.go as "asset 0 does not exist or has been deleted".
• FreezeAccount must not be zero (missing). This was previously caught by ledger/apply/asset.go as "asset not found in account".

[ASISBusiness]

Whilst going through the specs I've been doubling checking what's there and how go-algorand is doing it. For asset transactions it felt like there should be more than we currently had. So I've created this PR to see if it's worth relocating the existing acfg checks and introducing some new axfer/afrz wellFormed checks which I feel make sense.

acfg - moved:

• AssetName must not be longer than MaxAssetNameBytes. This was moved from data/transactions/transaction.go:Transaction.WellFormed().

• UnitName must not be longer than MaxAssetUnitNameBytes. This was moved from data/transactions/transaction.go:Transaction.WellFormed().

• URL must not be longer than MaxAssetURLBytes. This was moved from data/transactions/transaction.go:Transaction.WellFormed().

• Decimals must not be greater than MaxAssetDecimals. This was moved from data/transactions/transaction.go:Transaction.WellFormed().

axfer - new:

• XferAsset must not be zero (missing). This was previously caught by ledger/apply/asset.go as "asset 0 does not exist or has been deleted".

• AssetSender and AssetCloseTo cannot both be set. This was previously caught by ledger/apply/asset.go as "cannot close asset by clawback".

afrz - new:

• FreezeAsset must not be zero (missing). This was previously caught by ledger/apply/asset.go as "asset 0 does not exist or has been deleted".

• FreezeAccount must not be zero (missing). This was previously caught by ledger/apply/asset.go as "asset not found in account".

[jannotti]

I think this is great. I totally missed that we were doing some of these checks later in Transaction.WellFormed when I added the new wellFormed methods on the different kinds of fields.

I suppose one could debate whether the zero-address check on afrz is appropriate for wellFormed. I am ok with it, but one could make the argument that there's a logicsig out there somewhere that hashes to the zeroaddress, and maybe it can opt into an asset some day. I think we're allowed to discount that possibility.

I lean away from introducing all these:

var (
	errAssetIDCannotBeZero                = errors.New("asset ID cannot be zero")
	errFreezeAccountCannotBeEmpty         = errors.New("freeze account cannot be empty")
	errConfigAssetNameTooBig              = errors.New("transaction asset name too big")
	errConfigAssetUnitNameTooBig          = errors.New("transaction asset unit name too big")
	errConfigAssetURLTooBig               = errors.New("transaction asset url too big")
	errConfigAssetDecimalsTooHigh         = errors.New("transaction asset decimals is too high")
	errTransferCannotCloseAssetByClawback = errors.New("cannot close asset by clawback")
)

They're not exported (which is a goof thing, IMO), so it's not like there could be an equality check on them. I'm fine with putting the errors.New() directly on the error path.

To my mind, creating the variable advertises: "Callers can count on this thing, and compare errors to it." I hate that thinking because maybe we want to change them (maybe people complain the message should contain the big asset name, or the length, or whatever.)

[jannotti]

There are probably unit tests that ought to be moved to asset_test.go and target the individual wellFormed functions explicitly. (I bet you'll get some failures somewhere from the newly early failures for the stuff that came out of apply/...go.

[jannotti]

On the question of checking for the zeroAddress on afrz. Doing so implies we also ought to be checking zeroAddress on Sender (for all txn types). But that doesn't feel right to me.

EDIT:
OH!

	if tx.Sender.IsZero() {
		return fmt.Errorf("transaction cannot have zero sender")
	}

So yes, I think we reasonably have the check against zeroAddress in afrz - there's no way it can opt in.

[jannotti]

Uh oh, a 0 transfer of the 0 asset is legal! So, we can't make it illegal. It's legal because we don't look up the asset params for axfer (which is where the 0 asset would previously be detected). And we don't check opt-in for a 0 transfer.

[cusma]

Uh oh, a 0 transfer of the 0 asset is legal! So, we can't make it illegal. It's legal because we don't look up the asset params for axfer (which is where the 0 asset would previously be detected). And we don't check opt-in for a 0 transfer.

Currently, it is possible to send a 0 asset amount to a receiver who has not opted into the asset. I think we should not change this.

[cusma]

On the question of checking for the zeroAddress on afrz. Doing so implies we also ought to be checking zeroAddress on Sender (for all txn types). But that doesn't feel right to me.

EDIT: OH!

	if tx.Sender.IsZero() {
		return fmt.Errorf("transaction cannot have zero sender")
	}

So yes, I think we reasonably have the check against zeroAddress in afrz - there's no way it can opt in.

@nullun, just a note: when we discussed a possible retro-compatible way to implement an ASA global freeze, the ZeroAddress target of the afrz was the way to say "this is a global (and not local) freeze".

[jannotti]

On the question of checking for the zeroAddress on afrz. Doing so implies we also ought to be checking zeroAddress on Sender (for all txn types). But that doesn't feel right to me.
EDIT: OH!

	if tx.Sender.IsZero() {
		return fmt.Errorf("transaction cannot have zero sender")
	}

So yes, I think we reasonably have the check against zeroAddress in afrz - there's no way it can opt in.

@nullun, just a note: when we discussed a possible retro-compatible way to implement an ASA global freeze, the ZeroAddress target of the afrz was the way to say "this is a global (and not local) freeze".

I think that option would remain, even with this change. Obviously we would then allow it with that new meaning, but for now it's illegal, and this just changes where we detect it.

[jannotti]

FYI, this test failure:

=== FAIL: data/transactions/logic TestCurrentInnerTypes (0.06s)
    evalAppTxn_test.go:72: 
        	Error Trace:	/home/runner/work/go-algorand/go-algorand/data/transactions/logic/evalStateful_test.go:568
        	            				/home/runner/work/go-algorand/go-algorand/data/transactions/logic/evalStateful_test.go:513
        	            				/home/runner/work/go-algorand/go-algorand/data/transactions/logic/evalStateful_test.go:490
        	            				/home/runner/work/go-algorand/go-algorand/data/transactions/logic/evalAppTxn_test.go:72
        	Error:      	Error "logic eval error: asset ID cannot be zero. Details: app=888, pc=11" does not contain "insufficient balance"
        	Test:       	TestCurrentInnerTypes
        	Messages:   	Wrong error in EvalContract   1 itxn_begin
        	            	  2 pushbytes
        	            	  9 itxn_field
        	            	 11 itxn_submit
        	            	 12 pushint
        	            	  1 itxn_begin => <empty stack>
        	            	  2 pushbytes 0x6178666572 // "axfer" => (6178666572) 
        	            	  9 itxn_field Type => <empty stack>
        	            	 11 itxn_submit => <empty stack>
        	            	 11 asset ID cannot be zero

is an example where we are doing a 0 axfer to the 0 address in a test. (We're not explicitly testing that here, we're just relying on it to make writing this other test easy. So this failure is a true positive - it should keep working.

[nullun]

Uh oh, a 0 transfer of the 0 asset is legal! So, we can't make it illegal. It's legal because we don't look up the asset params for axfer (which is where the 0 asset would previously be detected). And we don't check opt-in for a 0 transfer.

For 0 amounts, putIn and takeOut immediately return, since it's not modifying account state either.

I wonder if updating the if-statement to if ax.XferAsset == basics.AssetIndex(0) && ax.AssetAmount != 0 { would satisfy the check?
It feels like I'm changing existing behaviour, but it could never actually been done since no one can opt-in to it. Meanwhile in other places like ledger/apply/asset.go:AssetConfig() we assume an ID of 0 is for creating a new asset, so such an asset would break other things already.

[jannotti]

I wonder if updating the if-statement to if ax.XferAsset == basics.AssetIndex(0) && ax.AssetAmount != 0 { would satisfy the check? It feels like I'm changing existing behaviour, but it could never actually been done since no one can opt-in to it. Meanwhile in other places like ledger/apply/asset.go:AssetConfig() we assume an ID of 0 is for creating a new asset, so such an asset would break other things already.

This extra check should be fine. The 0 axfer of the 0 asset or, in fact any asset number even if the asset doesn't exist, is legal today and surely someone has done it. Neither the sender nor receiver needs to be opted in. So we can't exclude 0 unless the amount is nonzero.

PS. 0 is special enough in Go that you can write if ax.XferAsset == 0 && ax.AssetAmount != 0

[jannotti]

They were already true, though they errored in `apply` rather than in well formed. So I believe this is safe.

…

On Wed, Aug 6, 2025, 3:45 PM Pavel Zbitskiy ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In data/transactions/asset.go <#6396 (comment)> : > + if ax.XferAsset == 0 && ax.AssetAmount != 0 { + return errors.New("asset ID cannot be zero") + } + + if !ax.AssetSender.IsZero() && !ax.AssetCloseTo.IsZero() { + return errors.New("cannot close asset by clawback") + } + sounds like these new restrictions should be consensus gated — Reply to this email directly, view it on GitHub <#6396 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADL7TZENEOXM2QV65ZPUYL3MJLL7AVCNFSM6AAAAACCWOENCSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTAOJTHE4DKNBZGE> . You are receiving this because you commented.Message ID: ***@***.***>

[nullun]

@jannotti whilst updating the tests I believe I came across a scenario where data/transactions/logic/ledger_test.go was incorrectly failing the transaction. d539e9e
I made a small change so it doesn't return early. Let me know if I've misunderstood something here.

[jannotti]

@jannotti whilst updating the tests I believe I came across a scenario where data/transactions/logic/ledger_test.go was incorrectly failing the transaction. d539e9e I made a small change so it doesn't return early. Let me know if I've misunderstood something here.

I think your change is correct, because it now errors if trying to opt-in to a non-existent asset. But you said it was "incorrectly failing a transaction". Wasn't the probably that it was incorrectly allowing the transaction (of opt-in of missing asset)?

[jannotti]

While I like the tests you added to evalAppTxn_test.go, it's also a bit strange to be testing these changes, first and foremost, in inner transactions. You should probably add something like:
func TestAppCallAccessWellFormed(t *testing.T), one for each of the asset txns.

[nullun]

you said it was "incorrectly failing a transaction". Wasn't the probably that it was incorrectly allowing the transaction (of opt-in of missing asset)?

Sorry, I meant it was failing the new test I added. The existing tests weren't incorrectly failing anything.

Essentially the new test I added in that commit was attempting to send 0 amount of an asset. Neither the sender nor receiver were opted in. But the error being returned was saying "Sender (...) not opted in to ...", rather than allowing the transaction.

While I like the tests you added to evalAppTxn_test.go, it's also a bit strange to be testing these changes, first and foremost, in inner transactions. You should probably add something like: func TestAppCallAccessWellFormed(t *testing.T), one for each of the asset txns.

That's fair. I was originally just updating the tests to check for the correct error message, but got a bit carried away and thought I'd include a test for the two new wellFormed errors. Is it OK to leave them there for now, and once I'm back (going away for a week) I'll create separate a separate WellFormed test outside of the inner transaction tests.