Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AVM: Allow immutable access to foreign app accounts #3994

Merged
merged 11 commits into from
May 23, 2022
3 changes: 3 additions & 0 deletions data/transactions/logic/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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_.

## Constants

Constants can be pushed onto the stack in two different ways:
Expand Down
3 changes: 3 additions & 0 deletions data/transactions/logic/README_in.md
Original file line number Diff line number Diff line change
Expand Up @@ -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_.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`txn.ForeignApplications` is _available_.
`txn.ForeignApplications` field is _available_.


## Constants

Constants can be pushed onto the stack in two different ways:
Expand Down
22 changes: 16 additions & 6 deletions data/transactions/logic/eval.go
Original file line number Diff line number Diff line change
Expand Up @@ -3524,12 +3524,12 @@ func opExtract64Bits(cx *EvalContext) error {
}

// accountReference yields the address and Accounts offset designated by a
// stackValue. If the stackValue is the app account or an account of an app in
// created.apps, and it is not be in the Accounts array, then len(Accounts) + 1
// is returned as the index. This would let us catch the mistake if the index is
// used for set/del. If the txn somehow "psychically" predicted the address, and
// therefore it IS in txn.Accounts, then happy day, we can set/del it. Return
// the proper index.
// stackValue. If the stackValue is the app account, an account of an app in
// created.apps, or an account of an app in foreignApps, and it is not in the
// Accounts array, then len(Accounts) + 1 is returned as the index. This would
// let us catch the mistake if the index is used for set/del. If the txn somehow
// "psychically" predicted the address, and therefore it IS in txn.Accounts,
// then happy day, we can set/del it. Return the proper index.

// If we ever want apps to be able to change local state on these accounts
// (which includes this app's own account!), we will need a change to
Expand Down Expand Up @@ -3558,6 +3558,16 @@ func (cx *EvalContext) accountReference(account stackValue) (basics.Address, uin
}
}

// Allow an address for an app that was provided in the foreign apps array.
if err != nil && cx.version >= appAddressAvailableVersion {
for _, appID := range cx.txn.Txn.ForeignApps {
foreignAddress := cx.getApplicationAddress(appID)
if addr == foreignAddress {
return addr, invalidIndex, nil
}
}
}

// this app's address is also allowed
if err != nil {
appAddr := cx.getApplicationAddress(cx.appID)
Expand Down
28 changes: 28 additions & 0 deletions data/transactions/logic/evalAppTxn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2907,3 +2907,31 @@ itxn_submit

TestApp(t, source, ep, "appl depth (8) exceeded")
}

func TestForeignAppAccountAccess(t *testing.T) {
partitiontest.PartitionTest(t)

ep, tx, ledger := MakeSampleEnv()
ledger.NewAccount(appAddr(888), 50_000)
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{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ApprovalProgram: TestProg(t, "int 1", AssemblerMaxVersion).Program,
ClearStateProgram: TestProg(t, "int 1", AssemblerMaxVersion).Program,
})

TestApp(t, `
itxn_begin
int pay
itxn_field TypeEnum
int 100
itxn_field Amount
txn Applications 1
app_params_get AppAddress
assert
itxn_field Receiver
itxn_submit
int 1
`, ep)
}
5 changes: 5 additions & 0 deletions data/transactions/logic/opcodes.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ const txnEffectsVersion = 6
// the Foreign arrays.
const createdResourcesVersion = 6

// appAddressAvailableVersion is the first version that allows access to the
// accounts of applications that were provided in the foreign apps transaction
// field.
const appAddressAvailableVersion = 7

// experimental-
const fidoVersion = 7 // base64, json, secp256r1

Expand Down
219 changes: 219 additions & 0 deletions ledger/internal/apptxn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3058,3 +3058,222 @@ check:
txns(t, l, eval, &fundA, &callA)
endBlock(t, l, eval)
}

func TestForeignAppAccountsAccessible(t *testing.T) {
partitiontest.PartitionTest(t)

genBalances, addrs, _ := ledgertesting.NewTestGenesis()
testConsensusRange(t, 32, 0, func(t *testing.T, ver int) {
dl := NewDoubleLedger(t, genBalances, consensusByNumber[ver])
defer dl.Close()

appA := txntest.Txn{
Type: "appl",
Sender: addrs[0],
ApprovalProgram: main(`
int 3
int 3
==
assert
`),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

appB := txntest.Txn{
Type: "appl",
Sender: addrs[0],
ApprovalProgram: main(`
itxn_begin
int pay; itxn_field TypeEnum
int 100; itxn_field Amount
txn Applications 1
app_params_get AppAddress
assert
itxn_field Receiver
itxn_submit
`),
}

dl.beginBlock()
dl.txgroup("", &appA, &appB)
vb := dl.endBlock()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

index0 := vb.Block().Payset[0].ApplicationID
index1 := vb.Block().Payset[1].ApplicationID

fund1 := txntest.Txn{
Type: "pay",
Sender: addrs[0],
Receiver: index1.Address(),
Amount: 1_000_000_000,
}
fund0 := fund1
fund0.Receiver = index0.Address()
fund1.Receiver = index1.Address()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


callTx := txntest.Txn{
Type: "appl",
Sender: addrs[2],
ApplicationID: index1,
ForeignApps: []basics.AppIndex{index0},
}

dl.beginBlock()
if ver <= 32 {
dl.txgroup("invalid Account reference", &fund0, &fund1, &callTx)
dl.endBlock()
return
}

dl.txgroup("", &fund0, &fund1, &callTx)
vb = dl.endBlock()

require.Equal(t, index0.Address(), vb.Block().Payset[2].EvalDelta.InnerTxns[0].Txn.Receiver)
require.Equal(t, uint64(100), vb.Block().Payset[2].EvalDelta.InnerTxns[0].Txn.Amount.Raw)
})
}

// While accounts of foreign apps are available in most context, they still
// cannot be used as mutable references; ie the accounts cannot be used by
// opcodes that modify local storage.
func TestForeignAppAccountsImmutable(t *testing.T) {
algoidurovic marked this conversation as resolved.
Show resolved Hide resolved
partitiontest.PartitionTest(t)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


appA := txntest.Txn{
Type: "appl",
Sender: addrs[0],
ApprovalProgram: main(`
int 3
int 3
==
assert
`),
}

appB := txntest.Txn{
Type: "appl",
Sender: addrs[0],
ApprovalProgram: main(`
txn Applications 1
app_params_get AppAddress
byte "X"
byte "ABC"
app_local_put
int 1
`),
}

eval := nextBlock(t, l)
txns(t, l, eval, &appA, &appB)
vb := endBlock(t, l, eval)
index0 := vb.Block().Payset[0].ApplicationID
index1 := vb.Block().Payset[1].ApplicationID

fund1 := txntest.Txn{
Type: "pay",
Sender: addrs[0],
Receiver: index1.Address(),
Amount: 1_000_000_000,
}
fund0 := fund1
fund0.Receiver = index0.Address()
fund1.Receiver = index1.Address()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same


callTx := txntest.Txn{
Type: "appl",
Sender: addrs[2],
ApplicationID: index1,
ForeignApps: []basics.AppIndex{index0},
}
eval = nextBlock(t, l)
txns(t, l, eval, &fund0, &fund1)
txn(t, l, eval, &callTx, "invalid Account reference")
endBlock(t, l, eval)
}

// In the case where the foreign app account is also provided in the
// transaction's account field, mutable references should be allowed.
func TestForeignAppAccountsMutable(t *testing.T) {
partitiontest.PartitionTest(t)

genBalances, addrs, _ := ledgertesting.NewTestGenesis()
testConsensusRange(t, 32, 0, func(t *testing.T, ver int) {
dl := NewDoubleLedger(t, genBalances, consensusByNumber[ver])
defer dl.Close()

appA := txntest.Txn{
Type: "appl",
Sender: addrs[0],
ApprovalProgram: main(`
itxn_begin
int appl
itxn_field TypeEnum
txn Applications 1
itxn_field ApplicationID
int OptIn
itxn_field OnCompletion
itxn_submit
`),
}

appB := txntest.Txn{
Type: "appl",
Sender: addrs[0],
ApprovalProgram: main(`
txn OnCompletion
int OptIn
==
bnz done
txn Applications 1
app_params_get AppAddress
assert
byte "X"
byte "Y"
app_local_put
done:
`),
LocalStateSchema: basics.StateSchema{
NumByteSlice: 1,
},
}

dl.beginBlock()
dl.txgroup("", &appA, &appB)
vb := dl.endBlock()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

index0 := vb.Block().Payset[0].ApplicationID
index1 := vb.Block().Payset[1].ApplicationID

fund1 := txntest.Txn{
Type: "pay",
Sender: addrs[0],
Receiver: index1.Address(),
Amount: 1_000_000_000,
}
fund0 := fund1
fund0.Receiver = index0.Address()
fund1.Receiver = index1.Address()

callA := txntest.Txn{
Type: "appl",
Sender: addrs[2],
ApplicationID: index0,
ForeignApps: []basics.AppIndex{index1},
}

callB := txntest.Txn{
Type: "appl",
Sender: addrs[2],
ApplicationID: index1,
ForeignApps: []basics.AppIndex{index0},
Accounts: []basics.Address{index0.Address()},
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


require.Equal(t, "Y", vb.Block().Payset[3].EvalDelta.LocalDeltas[1]["X"].Bytes)
})
}