Inner asset txns#2871
Conversation
This only changes the blocks, so there is still a lot of work to do so that external APIs see things properly. The REST API assumes that it can determine a creatble ID based on the index of a transaction in a block and the block's TxnCounter. That is no longer true, it must actually figure out how many inner txns it is skipping over.
This also sorts out inner txcounting, so that an innner transaction knows the current txncount as it executes, so it can get the right id if it creates.
jannotti
force-pushed
the
inner-asset-txns
branch
from
eb9e597 to
bae2859
Compare
algorandskiy
left a comment
algorandskiy
left a comment
There was a problem hiding this comment.
I like this one better. Probably need to add some e2e tests
| vb := l.endBlock(t, eval) | ||
|
|
||
| asaIndex := vb.blk.Payset[1].EvalDelta.InnerTxns[0].ConfigAsset | ||
| require.Equal(t, basics.AssetIndex(5), asaIndex) |
There was a problem hiding this comment.
Could you also pull out the params of the newly created asset and check its fields match the values set by the AVM? (Maybe this is better in an e2e test)
Also, I suppose it's not possible to actually transfer the newly created asset or use the asset_param_get op on it since it can't be in the foreign assets array? I wonder if we should make an exception in this case and allow the asset to be "available" anyway.
There was a problem hiding this comment.
WiIll do, on testing.
I was strongly considering adding a special case (as we did for the app address) that would allow the use of an asset that had been created in the same transaction (or txgroup?). But as for as I can tell, it's pretty much impossible to use, because no recipient could possibly be opted-in yet.
There was a problem hiding this comment.
Well theoretically the app could have another account rekeyed to it, so it could potentially opt that account into the new asset and perform a transfer.
The bigger issue I see is that there's currently no way to get the ID of the created asset. If we don't support this, there's no reason to support the special case of adding newly created assets to the "available" list.
(I suppose I'm ok with delaying the ID retrieval to a later update, since it's better to have acfg without that than no acfg at all.)
There was a problem hiding this comment.
I'd certainly like it too. Let me see if I can think of a decent interface that doesn't paint us into a corner. Ideas welcome.
There was a problem hiding this comment.
Perhaps
itxna Logs X
itxn ConfigAsset
(and attendant itxn NumLogs, itxnas)
These would all return results from previous tx_submit execution. And perhaps they become itxn_begin etc.
Codecov Report
@@ Coverage Diff @@
## master #2871 +/- ##
========================================
Coverage 47.34% 47.34%
========================================
Files 351 351
Lines 56523 56711 +188
========================================
+ Hits 26758 26852 +94
- Misses 26758 26839 +81
- Partials 3007 3020 +13
Continue to review full report at Codecov.
|
| // We are not allowing InnerTxns to have InnerTxns yet. Error if that happens. | ||
| for _, itx := range applyData.EvalDelta.InnerTxns { | ||
| if len(itx.ApplyData.EvalDelta.InnerTxns) > 0 { | ||
| return fmt.Errorf("inner transaction has inner transactions %v", itx) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure if this is really needed, since we don't have any way to trigger this; but I don't see how having this check would be a bad thing either.
tsachiherman
left a comment
tsachiherman
left a comment
There was a problem hiding this comment.
Overall looks great. I've added few minor code styling requests.
jannotti
force-pushed
the
inner-asset-txns
branch
from
a967d2a to
0e81400
Compare
|
Also, I did some local testing and pending transaction REST endpoint does not return the created asset ID when an inner txn creates an asset. See the implementation for |
Oh my. Thank you. |
jannotti
force-pushed
the
inner-asset-txns
branch
from
0e81400 to
7c81231
Compare
Test inner txn indexes
| // change the fee. Do it in itxn_submit. | ||
| fee = basics.SubSaturate(fee, *cx.FeeCredit) | ||
| } | ||
| cx.subtxn.Txn.Header = transactions.Header{ |
There was a problem hiding this comment.
Hi, a quick question. Should we assume the inner tx belongs to the same group/lease than the parent despite not present and should be added when calculating the ID of the inner tx?
There was a problem hiding this comment.
The inner txn has no id.
There was a problem hiding this comment.
Ahh ok, thanks for the info because, from the indexers and from AlgoExplorer point of view, we didn't know if these txs could be identified individually.
There was a problem hiding this comment.
Indexer may well put a fake id in its internal table in order to place itxns in the same table with top-level. But you would obtain the info pertaining to an itxn only by asking for the top-level, and receive a tree back.
There was a problem hiding this comment.
Yes, that's a good idea and will see with @winder the approach to show the same fake id.
There was a problem hiding this comment.
Using fake IDs may allow Indexer/AlgoExplorer users to search for an inner transaction easily using a regular TXID.
If we do that, we should agree on a definition for those fake txids. Maybe it could be something like digest(tx, parent id, offset)
There was a problem hiding this comment.
I don't love the fake txid thing. I hope we'll keep it an implementation detail and avoid confusing users.
There was a problem hiding this comment.
And what about adding an "inner transaction index" field to the subtxn. Node must check this field is not defined or zero when a TX is submitted. This plus the first/last round you already added, should do the work.
|
Closed in favor of #2883 |
9 participants
acfg and afrz inner transactions
This also sorts out inner txcounting, so that an innner transaction knows the current txncount as it executes, so it can get the right id if it creates.
Need some more tests, but so far we have some teal unit tests, and ledger level integration tests.