AVM: Teal macros #4737
AVM: Teal macros #4737
Conversation
More tests and label coverage
|
I am curious if @fergalwalsh or @jeapostrophe would care to use this in the TEAL they emit to make it more readable / auditable. |
Codecov Report
@@ Coverage Diff @@
## master #4737 +/- ##
==========================================
+ Coverage 54.62% 54.67% +0.05%
==========================================
Files 414 414
Lines 53514 53595 +81
==========================================
+ Hits 29231 29303 +72
- Misses 21860 21868 +8
- Partials 2423 2424 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This looks interesting indeed. It can certainly be nice for constants. For macro 'ops' it would be nice if we come up with a shared standard library of common macros (I see you have the starts of one in the tests) so that there is some consistency across the space. Otherwise it may actually make things more difficult for auditors. e.g. if my I'll try to spend some time playing with this and give feedback if I have any. |
| @@ -31,6 +31,7 @@ import ( | |||
| "sort" | |||
| "strconv" | |||
| "strings" | |||
There was a problem hiding this comment.
@jannotti Minor: Are macros worthy of a spec modification (https://github.com/algorand/go-algorand/blob/master/data/transactions/logic/README.md)?
I'm imagining something in the neighborhood of https://github.com/algorand/go-algorand/blob/master/data/transactions/logic/README.md#constants being extended.
There was a problem hiding this comment.
Yes, I suppose so. It's not really a spec change (of the protocol) but since that's where we document some assembler level stuff, it would be the right place to explain these as well. I'll add.
There was a problem hiding this comment.
I think we can merge without a spec mod, as they don't actually change the protocol.
|
I like it, the testing is good, the recheck is funny. It does feel ripe for getting overlooked if additional reserved strings are added. also I wonder if enforcing some name prefix would lead to improved readability, or maybe it could just be convention. |
| if isTxnType || isOnCompletion { | ||
| return fmt.Errorf("Named constants cannot be used as macro names: %s", macroName) | ||
| } | ||
| if _, ok := pseudoOps[macroName]; ok { |
There was a problem hiding this comment.
Note that if we introduce a new pseudo op that happens to have the same name as an existing program's macro, their code will fail to assemble, regardless of whether they've increased their pragma version or not (since pseudo ops aren't versioned). This also applies to txn and on completion types, but I expect new values for these to be much rarer.
I don't have a suggestion to improve the situation, I just wanted to point out this relationship.
There was a problem hiding this comment.
Good point. I think we would probably introduce versioning to those names if/when there were newly introduced values.
There was a problem hiding this comment.
Though I suppose we have not versioned pseudo-ops in the past, since they are compiled down to normal opcodes, and thus don't require versioning. We'll just have to keep this in mind and decide what to do then.
Adds simple:
style constants, and more complex:
style macros.