-
Notifications
You must be signed in to change notification settings - Fork 524
Add GroupID global to TEAL #2649
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
Conversation
| int 32 | ||
| bzero | ||
| == | ||
| && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we should also have a test where GroupID is not empty...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be doable with some hacking around in evalStateful_test.go, which builds up some groups with two txns in them.
|
That definitely looks like something missing that should be in there, but I kinda feel like this should be a |
|
It seems to me that |
|
It's absolutely debatable, since we've (mostly) used global to mean "Truly global - things about the blockchain" and txn to mean "About a single txn within the group". For me (and zach, it seems), the decider was GroupSize, which we did put under global. It seemed right to put GroupID and GroupSize in the same place. And yet, "grp" is indeed truly in the individual txns, so it seems like you should be able to get it from there. Honestly, it seems weird, but I could imagine allowing it from both. |
| CreatorAddress, globalV5TestProgram, | ||
| EvalStateful, CheckStateful, | ||
| }, | ||
| 6: {GroupID, globalV5TestProgram, Eval, Check}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why does it work, this assumes assembler version 6 but the field defined as version 5 constant
{GroupID, StackBytes, modeAny, 5},
This means the test is never called. I'll submit a PR checking len(tests) <= AssemblerMaxVersion
algorandskiy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting two changes:
- Add non-empty group id test
- Rebase on master -
TestGlobalwill fail because of non-existing version 6
|
Redone as #2838 to deal with conflicts. |
Summary
Adds the global field
GroupIDto retrieve the group of the transaction. This follows the pattern established forGroupSize.Test Plan
Added/ran unit tests.