Apps: txn.Access list for access to more resources #6286
Conversation
|
This shouldn't need a new AVM version, correct? Since AVM9 we've had group resource sharing, which means an app has no way of knowing what resources are available. Although I suppose it could check if it's an outer call and check the rest of the group, but seems highly improbable to account for all the sharing rules even if an app wanted to do that. |
|
I think you're asking "will only new programs, AVM v12, be able to use this?" I think it should be ok to let old programs use this, including letting these resources be accessed by other programs in the same group that are a low version. I'm pretty sure we made the same decision with resource pooling. After the consensus upgrade, programs suddenly got access to things they didn't "know" they had access to by looking into the arrays. I'll have to confirm whether we put in any limitations. For example, I think it would be unsafe to let v3 (!) programs see extra ASAs. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6286 +/- ##
==========================================
+ Coverage 49.81% 50.72% +0.90%
==========================================
Files 356 655 +299
Lines 64417 111179 +46762
==========================================
+ Hits 32091 56392 +24301
- Misses 30970 51921 +20951
- Partials 1356 2866 +1510 ☔ View full report in Codecov by Sentry. |
|
Right that's what I would think as well. The main reason I ask is because if we don't require a specific AVM version existing applications, even if immutable, can leverage this feature by simply updating their client-side code. |
This commit ensures that assets are made available properly with tx.Access
Having worked on it some more: Yes, any transaction can use As for sharing, we only enabled sharing for v9 programs and higher, so I made that true here as well. Your v9 or higher programs will be able to see the resources made available from other transactions, whether those transactions made the resources available with the "old-style" foreign arrays or with Like |
jannotti
force-pushed
the
access-list
branch
5 times, most recently
from
98e1981 to
1573e6b
Compare
Check out this beauty, co-authored by chatgpt.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a unified Access list for application calls, replacing the old separate foreign-arrays and allowing more than the previous 8 total references. It updates the Go SDK (libgoal) to accept a new RefBundle type in all app-transaction constructors, implements logic to translate that bundle into either legacy foreign arrays or the new access list, and adjusts TEAL programs and integration tests to exercise and validate the new behavior.
- Introduced
RefBundleand updatedMakeUnsignedApp*Txmethods inlibgoal/transactions.go - Added
attachAccessList/attachForeignRefsroutines to populate transactions fromRefBundle - Bumped TEAL versions and updated e2e scripts/tests to use
--accessand expect new “unavailable” errors
Reviewed Changes
Copilot reviewed 52 out of 53 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| util/fn.go | add generic Map/MapErr helpers |
| test/scripts/e2e_subs/tealprogs/xappreads.teal | bump to TEAL v9, replace failure labels with assert |
| test/scripts/e2e_subs/tealprogs/assets-escrow9.teal | new TEAL program demonstrating tx.Access usage |
| test/scripts/e2e_subs/shared-resources.py | update error substrings to “unavailable” |
| test/scripts/e2e_subs/e2e-app-x-app-reads.sh | revise CLI tests to use --access and expect new errors |
| test/scripts/e2e_subs/e2e-app-simulate.sh | update expected failure message to “unavailable Account” |
| test/scripts/e2e_subs/app-assets.sh | add exit-on-failure logic and consistent quoting |
| test/scripts/e2e_subs/app-assets-access.sh | new end-to-end asset tests with pervasive --access |
| test/e2e-go/upgrades/application_support_test.go | update calls to use libgoal.RefBundle{} |
| test/e2e-go/restAPI/simulate/simulateRestAPI_test.go | adjust simulation REST tests for new RefBundle signature |
| test/e2e-go/restAPI/other/appsRestAPI_test.go | adapt REST API app-creation tests to new RefBundle |
| test/e2e-go/features/transactions/application_test.go | refactor feature tests to use RefBundle in app calls |
| test/e2e-go/features/transactions/app_pages_test.go | update extra-pages app-create/update tests for RefBundle |
| test/e2e-go/features/transactions/accountv2_test.go | adjust account-v2 tests to new app-call signature |
| test/e2e-go/features/accountPerf/sixMillion_test.go | update performance tests for app-call ref changes |
| shared/pingpong/pingpong.go | switch pingpong to RefBundle and use slices |
| shared/pingpong/accounts.go | update pingpong account flows to new app-call API |
| scripts/export_sdk_types.py | extend export script to include new resource types |
| libgoal/transactions.go | introduce RefBundle, attachReferences, new access/foreign logic |
| libgoal/libgoal_test.go | add unit tests for attachForeignRefs and attachAccessList |
| ledger/simulation/simulation_eval_test.go | update simulation tests to use basics.BoxRef |
| ledger/simulation/resources.go | switch ResourceTracker.Boxes to map[basics.BoxRef] |
| ledger/boxtxn_test.go | bump box quota version and adjust budget checks |
| ledger/apptxn_test.go | typo fix in comment and update expected “unavailable” message |
| ledger/apply/application_test.go | remove obsolete tests, add t.Parallel() |
| ledger/apply/application.go | adjust checkPrograms to new gi parameter |
| data/txntest/txn.go | add Access []ResourceRef to Txn stub |
| data/transactions/msgp_gen_test.go | add msgpack tests for HoldingRef, LocalsRef, ResourceRef |
| data/transactions/logic/resources_test.go | update expected “unavailable Local State” formatting |
| data/transactions/logic/resources.go | use basics.BoxRef and implement fillApplicationCallAccess |
| data/transactions/logic/export_test.go | export new helpers for full-app tests |
| data/transactions/logic/eval_test.go | replace convertSlice with util.Map and import util |
| // that can be attached to a single app call. | ||
| // maximum number of "foreign references" (accounts, asa, app, boxes) that | ||
| // can be attached to a single app call. Modern transactions can use | ||
| // MaxAccess references in txn.Access to access more. |
There was a problem hiding this comment.
is this supposed to refer to MaxAppAccess?
| // w/ AssetSender) even if the Sender holding of the asset is not | ||
| // available. This parameters can be removed and assumed true after the | ||
| // first consensus release in which it is set true. | ||
| EnableInnerClawbackWithoutSenderHolding bool |
There was a problem hiding this comment.
IDEA: These parameters that are 'future-removable' - perhaps we should prefix them with 'Temp' or similar?
| @@ -1425,6 +1435,12 @@ func initConsensusProtocols() { | |||
| vFuture.EnableAppVersioning = true // if not promoted when v12 goes into effect, update logic/field.go | |||
| vFuture.EnableSha512BlockHash = true | |||
|
|
|||
| // txn.Access work | |||
| vFuture.MaxAppTxnAccounts = 8 // Accounts are no worse than others, they should be the same | |||
There was a problem hiding this comment.
Referring to the v24 comment, the idea is that we've capped with MaxAppAccess below, allowing us to loosen up here. Why not put all of them at 16?
There was a problem hiding this comment.
This 8 is only to bring Accounts inline with ForeignApps and ForiegnAssets, which are also 8. We need to keep these low, because of their cross-product implications. But Accounts was artificially low because a was dumb, years ago. It is nicer to have them all the same (but still lower than allowed in the access list).
Think of this as unrelated to tx.Access. (but I'm cramming it into this PR because it's tiny, and in the same vicinity)
data/transactions/application.go
Outdated
| return 0, string(br.Name), nil | ||
| case br.Index <= uint64(len(access)): // 1-based | ||
| app = access[br.Index-1].App | ||
| if app == 0 { |
There was a problem hiding this comment.
because this implies not existing?
There was a problem hiding this comment.
The box claims that it's for the app in "slot" br.Index of the access list. But that ResourceRef has nothing in .App , so that ResourceRef is not, in fact, an app. Maybe it's an Account or Assets, etc.
I have an idea to make this a little clearer.
| @@ -5211,7 +5219,10 @@ func (cx *EvalContext) assignAsset(sv stackValue) (basics.AssetIndex, error) { | |||
| // transaction (axfer,acfg,afrz), but not for holding lookups or assignments to | |||
| // an inner static array. | |||
| func (cx *EvalContext) availableAsset(aid basics.AssetIndex) bool { | |||
| // Ensure that aid is in Foreign Assets | |||
| // Check if aid is in an access array | |||
There was a problem hiding this comment.
Access does not have the 0 special case like the other slices, so this should all work.
| input = input.replace("Boxes []BoxRef", "BoxReferences []BoxReference") | ||
| input = re.sub("Box\\s+BoxRef", "Box BoxReference", input) |
There was a problem hiding this comment.
how did things look running this locally?
| @@ -1154,7 +1154,7 @@ int 1 | |||
|
|
|||
| // create the app | |||
| appTx, err = client.MakeUnsignedAppCreateTx( | |||
| transactions.OptInOC, approvalOps.Program, clearstateOps.Program, schema, schema, nil, nil, nil, nil, nil, 0) | |||
| transactions.OptInOC, approvalOps.Program, clearstateOps.Program, schema, schema, nil, libgoal.RefBundle{}, 0) | |||
There was a problem hiding this comment.
Works as long as we run against vfuture, would fail on current right?
There was a problem hiding this comment.
I think this would work on both, since it uses no references. RefBundle is just a way to group those resources into one argument. But here it's empty (and the args were nil)
| @@ -128,6 +128,7 @@ var passThruSource = main(` | |||
| `) | |||
|
|
|||
| const boxVersion = 36 | |||
| const boxQuotaBumpVersion = 41 | |||
There was a problem hiding this comment.
We're currently at v40, vFuture == 41. (And when vFuture is bumped, we should be releasing v41 which will have the quota bump)
Co-authored-by: Gary Malouf <[email protected]>
| // update, we should remove it from consensus params and assume it's true in | ||
| // the next release. It only needs to be in there so that it gates the | ||
| // beahvior change in the release it first appears. | ||
| if !cx.Proto.EnableInnerClawbackWithoutSenderHolding || tx.AssetSender.IsZero() { |
There was a problem hiding this comment.
Similar comment elsewhere; these 'use once and move on' params could maybe go by a special prefix or suffix..
There was a problem hiding this comment.
I think I like the idea.
Will it cause any problems in Go SDK when we remove the flag? We export the consensusparams struct to the Go SDK. Does Go json reading fail when there's a mismatch of json to the struct? Or just leave fields zero?
|
Honestly not sure, I would hope it would leave it blank (for other structs,
we add new fields before they are necessarily present on the algod side).
…On Thu, Jul 10, 2025 at 1:38 PM John Jannotti ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In data/transactions/logic/resources.go
<#6286 (comment)>
:
> }
return nil
}
func (cx *EvalContext) allowsAssetTransfer(hdr *transactions.Header, tx *transactions.AssetTransferTxnFields) error {
- err := cx.requireHolding(hdr.Sender, tx.XferAsset)
- if err != nil {
- return fmt.Errorf("axfer Sender: %w", err)
+ // After EnableInnerClawbackWithoutSenderHolding appears in a consensus
+ // update, we should remove it from consensus params and assume it's true in
+ // the next release. It only needs to be in there so that it gates the
+ // beahvior change in the release it first appears.
+ if !cx.Proto.EnableInnerClawbackWithoutSenderHolding || tx.AssetSender.IsZero() {
I think I like the idea.
Will it cause any problems in Go SDK when we remove the flag? We export
the consensusparams struct to the Go SDK. Does Go json reading fail when
there's a mismatch of json to the struct? Or just leave fields zero?
—
Reply to this email directly, view it on GitHub
<#6286 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHP3U2P6S5O5OODFPQTWX33H2QKBAVCNFSM6AAAAABZ23HDOCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTAMBWGY3DQNZYGU>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
And make the entire decription clearer, especially for holdings/locals.
Co-authored-by: Copilot <[email protected]>
| later application calls in the group, whether those application | ||
| calls are top-level or inner. | ||
|
|
||
| * v9 and later applications may use the `txn.Access` list instead of |
There was a problem hiding this comment.
Does the top level transaction alone determine the availability in all inners? i.e. what if an inner involves a pre-v9 application, etc...
There was a problem hiding this comment.
It's the version that's running that matters. So if a v5 calls a v9, the v9 will have access to group shared resources.
If v9 calls v7, it must pass the resources down in foreign-array fields when it creates the inner transaction, because it will only have access to locally available stuff.
| func processAppInputFile() (args [][]byte, accounts []string, foreignApps []uint64, foreignAssets []uint64, boxes []transactions.BoxRef) { | ||
| func getAppInputsFromFile() appCallInputs { | ||
| reportWarnf("Using a JSON app input file is deprecated and will be removed soon. Please speak up if the feature matters to you.") | ||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
that's one way to encourage folks to change!
There was a problem hiding this comment.
It's slightly friendlier than my preferred way: delete the code.
| @@ -54,6 +55,12 @@ const ( | |||
| // can contain. Its value is verified against consensus parameters in | |||
| // TestEncodedAppTxnAllocationBounds | |||
| encodedMaxBoxes = 32 | |||
|
|
|||
| // encodedMaxAcces sets the allocation bound for the maximum number of | |||
There was a problem hiding this comment.
comment nit: encodedMaxAcces -> encodedMaxAccess
| contract present in the `txn.ForeignApplications` field is | ||
| _available_. | ||
|
|
||
| * in v4 and above applications, Holdings and Locals are _available_ |
There was a problem hiding this comment.
nit: it's hard to follow. Sorting by version number would help a bit.
| // that can be attached to a single app call. | ||
| // maximum number of "foreign references" (accounts, asa, app, boxes) that | ||
| // can be attached to a single app call. Modern transactions can use | ||
| // MaxAccess references in txn.Access to access more. |
There was a problem hiding this comment.
| // MaxAccess references in txn.Access to access more. | |
| // MaxAppAccess references in txn.Access to access more. |
Summary
Today, app developers find it frustrating that they can only list 8 resources in a transaction. This number is artificially low because there are rules that allow access to many more than 8 items when, for example, 4 account and 4 apps are listed.
This PR introduced a single unified
Accessfield on app calls, which contains all accounts, apps, asas, and boxes that the transaction can touch. Because no extra rules allow access to extra resources, we can expand the allowable size of such a list. 32 seems likely. (edit: currently thinking 16, but also upping the box quota per reference to 2k)This will probably increase performance, since it will now be reasonable to perform perfect prefetching of all resources an app call might touch.
This PR also augments
goalto create these transactions if the--accessflag is used. It still needs e2e_subs tests that exercise it. changing back to draft until I write thoseThis PR does not actually implement the improved pre-fetching. Should it?
Test Plan