Skip to content

Conversation

@jannotti
Copy link
Contributor

txn opcodes should not be able to look at the current or future "effects" fields

@jannotti jannotti requested a review from jasonpaulos January 28, 2022 19:41
@jannotti jannotti self-assigned this Jan 28, 2022

itxn := &cx.Txn.EvalDelta.InnerTxns[len(cx.Txn.EvalDelta.InnerTxns)-1]
sv, err := cx.txnFieldToStack(itxn, fs, arrayFieldIdx, 0, true)
sv, err := cx.opTxnImpl(0, srcGroup, field, ai, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be srcInner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes!

Comment on lines 2272 to 2274
const srcGroup txnSource = 0
const srcInner txnSource = 1
const srcInnerGroup txnSource = 2
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
const srcGroup txnSource = 0
const srcInner txnSource = 1
const srcInnerGroup txnSource = 2
const (
srcGroup txnSource = 0
srcInner = 1
srcInnerGroup = 2
)

nit: this would be more recognizable as an enum like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll even go full on iota on it

// int(gi) is safe because gi < len(group). Slices in Go cannot exceed `int`
sv, err = cx.txnFieldToStack(tx, fs, ai, int(gi), src != srcGroup)
if err != nil {
cx.err = err
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: there's inconsistent use of cx.err. This check sets it, but all other checks in this function don't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. oddly enough that ends up having the exact same effect, but was certainly not intended

Next up, eliminate this cx.err assignment thing in all the opcodes.

@jannotti jannotti merged commit 84b52e7 into algorand:feature/contract-to-contract Jan 28, 2022
@jannotti jannotti deleted the past-effects-only branch January 28, 2022 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants