AVM: Add a lot of type annotations to opcodes #5902
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This allows the assembler to complain about clear mistakes, but maybe
more importantly, it improves the specs/documentation. We can now
document all the required byte lengths, even if they are strange, but
using "b{33}" in `proto()` call to require a 33 byte input.
But we also just missed a bunch of opportunities to annotate opcodes
in the past. I tried to consider ever single "b" in opcodes.go.
things to consider in review:
1) This should not change any behavior of assembled code. If it does,
that's a bug.
2) This _could_ make something that used to assemble stop
assembling. Probably, that means the author has a bug. If they really
want to call these opcodes with the wrong sized inputs, the can, using
`#pragma typetrack false`
3) Should we eliminate the existing single character names in proto()
that are only used to indicate a byte length? For example
```
{0x84, "ed25519verify_bare", opEd25519VerifyBare, proto("b63:T"), 7, costly(1900)},
```
could now be written
```
{0x84, "ed25519verify_bare", opEd25519VerifyBare, proto("b{64}b{32}:T"), 7, costly(1900)},
```
Perhaps that is clearer.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #5902 +/- ##
==========================================
- Coverage 55.97% 55.95% -0.02%
==========================================
Files 477 477
Lines 67463 67476 +13
==========================================
- Hits 37761 37755 -6
- Misses 27149 27162 +13
- Partials 2553 2559 +6 ☔ View full report in Codecov by Sentry. |
jasonpaulos
reviewed
Jan 11, 2024
jasonpaulos
previously approved these changes
Jan 16, 2024
jasonpaulos
approved these changes
Jan 16, 2024
algorandskiy
approved these changes
Jan 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows the assembler to complain about clear mistakes, but maybe more importantly, it improves the specs/documentation. We can now document all the required byte lengths, even if they are strange, by using "b{33}" in
proto()call to require a 33 byte input.But we also just missed a bunch of opportunities to annotate opcodes in the past. I tried to consider every single "b" in opcodes.go.
things to consider in review:
This should not change any behavior of assembled code. If it does, that's a bug.
This could make something that used to assemble stop assembling. Probably, that means the author has a bug. If they really want to call these opcodes with the wrong sized inputs, the can, using
#pragma typetrack falseSummary
Test Plan