Skip to content

Conversation

@zlangley
Copy link
Contributor

Summary

Adds the global field GroupID to retrieve the group of the transaction. This follows the pattern established for GroupSize.

Test Plan

Added/ran unit tests.

int 32
bzero
==
&&
Copy link
Contributor Author

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...

Copy link
Contributor

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.

@brianolson
Copy link
Contributor

That definitely looks like something missing that should be in there, but I kinda feel like this should be a txn field (fields.go TxnField) because Group is a field on a txn in the data model (it will be the same for everything in a group, so global is also kinda true, but I'd put it in txn)

@zlangley
Copy link
Contributor Author

It seems to me that Group being a transaction field is just an implementation detail; logically, the Group ID is a property of the group, not the transaction, no? So my instinct is that GroupID belongs in the same scope as the other group properties (e.g., GroupSize), but if you still feel strongly the other way I can change it.

@jannotti
Copy link
Contributor

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},
Copy link
Contributor

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

Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

Requesting two changes:

  1. Add non-empty group id test
  2. Rebase on master - TestGlobal will fail because of non-existing version 6

@jannotti
Copy link
Contributor

jannotti commented Sep 4, 2021

Redone as #2838 to deal with conflicts.

@jannotti jannotti closed this Sep 4, 2021
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.

4 participants