AVM: Allow immutable access to foreign app accounts #3994

algoidurovic · 2022-05-17T17:11:55Z

Summary

The AVM allows access to a few different categories of accounts: the current app's, those belonging to apps created previously in the tx group, and those provided by the accounts tx field. The PR expands this set to include the accounts belonging to the apps provided by the foreign_apps tx field.

Test Plan

The standard unit tests for eval.go don't have access to a sufficiently featured ledger so the tests were added to ledger/internal instead.

jannotti

The different flag is a needed change. And testing before and after seems important, but doesn't have to be done using testConsensusRange (but you'll thank me, it's easier)

data/transactions/logic/eval.go

ledger/internal/apptxn_test.go

jannotti · 2022-05-17T17:57:04Z

Could you also update the Resource availability section in README_in.md (and regenerate specs) to document the change? It should just be a new bullet about v7.

codecov · 2022-05-17T18:11:57Z

Codecov Report

Merging #3994 (2166891) into master (aae5ef2) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3994      +/-   ##
==========================================
- Coverage   54.49%   54.43%   -0.07%     
==========================================
  Files         390      390              
  Lines       48500    48504       +4     
==========================================
- Hits        26430    26402      -28     
- Misses      19843    19876      +33     
+ Partials     2227     2226       -1

Impacted Files	Coverage Δ
data/transactions/logic/opcodes.go	`84.86% <ø> (ø)`
data/transactions/logic/eval.go	`89.56% <100.00%> (+0.01%)`	⬆️
ledger/roundlru.go	`90.56% <0.00%> (-5.67%)`	⬇️
network/wsPeer.go	`65.83% <0.00%> (-3.06%)`	⬇️
crypto/merkletrie/trie.go	`66.42% <0.00%> (-2.19%)`	⬇️
crypto/merkletrie/node.go	`91.62% <0.00%> (-1.87%)`	⬇️
node/node.go	`23.19% <0.00%> (-1.85%)`	⬇️
ledger/acctupdates.go	`68.77% <0.00%> (-0.80%)`	⬇️
data/transactions/verify/txn.go	`45.02% <0.00%> (+0.86%)`	⬆️
data/abi/abi_type.go	`88.62% <0.00%> (+0.94%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aae5ef2...2166891. Read the comment docs.

… app_accounts

jannotti

Only the typo in the spec really needs to change, and that's trivial.

The others are a little paranoia, and tidying.

Looks good, I like the attempt to actually use the account in an inner pay.

jannotti · 2022-05-22T01:56:43Z

data/transactions/logic/README_in.md

@@ -180,6 +180,9 @@ _available_.
   associated account of a contract that was created earlier in the
   group is _available_.

+ * Since v7, the account associated with any contract present in the
+   `txn.ForeignApplications` is _available_.


Suggested change

`txn.ForeignApplications` is _available_.

`txn.ForeignApplications` field is _available_.

jannotti · 2022-05-22T02:03:24Z

data/transactions/logic/evalAppTxn_test.go

+	tx.ForeignApps = []basics.AppIndex{basics.AppIndex(2)}
+
+	// This app looks itself up in the ledger, so we need to put it in there.
+	ledger.NewApp(tx.Sender, 2, basics.AppParams{


Could you change this 2 to a higher number? I'm just being a little paranoid because I want it to be obvious that the 2 is the appID of the app, and not a slot number in ForeignApps.

Also I think that comment is a cut-n-paste. The app isn't looking itself up. The app being tested looks up this app.

jannotti · 2022-05-22T02:05:36Z

ledger/internal/apptxn_test.go

+		dl.beginBlock()
+		dl.txgroup("", &appA, &appB)
+		vb := dl.endBlock()


Suggested change

dl.beginBlock()

dl.txgroup("", &appA, &appB)

vb := dl.endBlock()

vb := dl.fullBlock(&appA, &appB)

unless they actually need to be in a txgroup, rather than just run sequentially.

jannotti · 2022-05-22T02:07:24Z

ledger/internal/apptxn_test.go

+		}
+		fund0 := fund1
+		fund0.Receiver = index0.Address()
+		fund1.Receiver = index1.Address()


This seems to rewriting the same value, as fund1 hasn't changed, right?

jannotti · 2022-05-22T02:10:52Z

ledger/internal/apptxn_test.go

+	}
+	fund0 := fund1
+	fund0.Receiver = index0.Address()
+	fund1.Receiver = index1.Address()


jannotti · 2022-05-22T02:14:16Z

ledger/internal/apptxn_test.go

+		dl.beginBlock()
+		dl.txgroup("", &fund0, &fund1, &callA, &callB)
+		vb = dl.endBlock()


Suggested change

dl.beginBlock()

dl.txgroup("", &fund0, &fund1, &callA, &callB)

vb = dl.endBlock()

vb = dl.fullBlock(&fund0, &fund1, &callA, &callB))

unless they actually need to be in a txgroup, rather than just run sequentially.

jannotti · 2022-05-22T02:14:55Z

ledger/internal/apptxn_test.go

+		dl.beginBlock()
+		dl.txgroup("", &appA, &appB)
+		vb := dl.endBlock()


Suggested change

dl.beginBlock()

dl.txgroup("", &appA, &appB)

vb := dl.endBlock()

vb := dl.fullBlock(&appA, &appB)

unless they actually need to be in a txgroup, rather than just run sequentially.

jannotti · 2022-05-22T02:20:24Z

ledger/internal/apptxn_test.go

+			ApprovalProgram: main(`
+int 3
+int 3
+==
+assert
+`),


You can actually leave this out and txntest.Txn will supply a passing ApprovalProgram.

jannotti · 2022-05-22T02:22:05Z

ledger/internal/apptxn_test.go

+	genBalances, addrs, _ := ledgertesting.NewTestGenesis()
+	l := newTestLedger(t, genBalances)
+	defer l.Close()


It would be nice if this was also testConsensusRange so that it tests all the versions, starting with the first that works, through vFuture as we add more and more versions.

jannotti

Looks good, thanks for those consensusRange changes. They make me feel much more confident as we change things in the future.

jasonpaulos

Looks good to me

MetaB0y · 2022-08-14T08:44:35Z

This is great! Thank you for implementing!

allow foreign app accounts to be accessed (immutably)

f79d2dd

algoidurovic added the Enhancement label May 17, 2022

algoidurovic linked an issue May 17, 2022 that may be closed by this pull request

Smart contracts should be given access to accounts for apps specified in the foreign apps array #3723

Closed

algoidurovic added 2 commits May 17, 2022 13:19

test for immutability

fe7898e

update comment

2804373

algoidurovic requested a review from jannotti May 17, 2022 17:23

jannotti requested changes May 17, 2022

View reviewed changes

data/transactions/logic/eval.go Outdated Show resolved Hide resolved

ledger/internal/apptxn_test.go Outdated Show resolved Hide resolved

ledger/internal/apptxn_test.go Show resolved Hide resolved

algoidurovic added 7 commits May 17, 2022 15:34

improve test coverage and guard new behavior with version flag

4f81772

generate readme and add clarifying comment

45e46fd

remove generated files

380cca3

add mutable account reference test

b9e160c

Merge branch 'master' of https://github.com/algorand/go-algorand into…

a10d253

… app_accounts

Merge branch 'master' of https://github.com/algorand/go-algorand into…

26a0660

… app_accounts

Merge branch 'master' of https://github.com/algorand/go-algorand into…

10aa9df

… app_accounts

jannotti requested changes May 22, 2022

View reviewed changes

jannotti reviewed May 22, 2022

View reviewed changes

remove unnecessary code in tests

2166891

jannotti approved these changes May 23, 2022

View reviewed changes

jasonpaulos approved these changes May 23, 2022

View reviewed changes

jannotti merged commit b3e19e7 into algorand:master May 23, 2022

This was referenced May 27, 2022

[Not Merged] go-algorand 3.7.0-beta Release PR #4049

Closed

Release: go-algorand 3.7.0-beta Release PR #4050

Merged

Algo-devops-service mentioned this pull request Jun 3, 2022

go-algorand 3.7.0-stable Release PR #4071

Merged

This was referenced Jun 8, 2022

go-algorand 3.7.1-stable Release PR #4091

Merged

go-algorand 3.7.2-stable Release PR #4118

Merged

algoanne mentioned this pull request Aug 10, 2022

go-algorand 3.9.0-beta Release PR #4383

Merged

onetechnical mentioned this pull request Sep 1, 2022

go-algorand 3.9.2-stable Release PR #4506

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AVM: Allow immutable access to foreign app accounts #3994

AVM: Allow immutable access to foreign app accounts #3994

algoidurovic commented May 17, 2022

jannotti left a comment

jannotti commented May 17, 2022

codecov bot commented May 17, 2022 •

edited

Loading

jannotti left a comment

jannotti May 22, 2022

jannotti May 22, 2022

jannotti May 22, 2022

jannotti May 22, 2022

jannotti May 22, 2022

jannotti May 22, 2022

jannotti May 22, 2022

jannotti May 22, 2022

jannotti May 22, 2022

jannotti left a comment

jasonpaulos left a comment

MetaB0y commented Aug 14, 2022 •

edited

Loading

	`txn.ForeignApplications` is _available_.
	`txn.ForeignApplications` field is _available_.

AVM: Allow immutable access to foreign app accounts #3994

AVM: Allow immutable access to foreign app accounts #3994

Conversation

algoidurovic commented May 17, 2022

Summary

Test Plan

jannotti left a comment

Choose a reason for hiding this comment

jannotti commented May 17, 2022

codecov bot commented May 17, 2022 • edited Loading

Codecov Report

jannotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannotti left a comment

Choose a reason for hiding this comment

jasonpaulos left a comment

Choose a reason for hiding this comment

MetaB0y commented Aug 14, 2022 • edited Loading

codecov bot commented May 17, 2022 •

edited

Loading

MetaB0y commented Aug 14, 2022 •

edited

Loading