Ensure disassemble/reassemble cycle works in testProg. #2745
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2745 +/- ##
==========================================
- Coverage 47.14% 47.07% -0.07%
==========================================
Files 349 349
Lines 56322 56329 +7
==========================================
- Hits 26553 26517 -36
- Misses 26800 26833 +33
- Partials 2969 2979 +10
Continue to review full report at Codecov.
|
| @@ -1034,7 +1034,7 @@ func TestGlobal(t *testing.T) { | |||
| addr, err := basics.UnmarshalChecksumAddress(testAddr) | |||
| require.NoError(t, err) | |||
| ledger.creatorAddr = addr | |||
| for v := uint64(0); v <= AssemblerMaxVersion; v++ { | |||
| for v := uint64(1); v <= AssemblerMaxVersion; v++ { | |||
There was a problem hiding this comment.
why is it changed to 1? 0 is used to be a valid version for TEAL 1
There was a problem hiding this comment.
The trouble is that when AssembleStringWithVersion is given 0 as the version, the actual assembled version is 1, and so #pragma version 1 is generated in the disassembly. But then AssembleStringWithVersion(dissassembly, 0) fails to reassemble it, because the source specifies 1 when the assembler was explicitly called with 0. We could probably make all this work, but AssembleStringWithVersion is not actually used outside of testing, so it seems pointless to make it clever about accepting 1 when called with 0 (which might be reasonable!). The only public use invokes it with assembleNoVersion. None of the other versioning tests test against 0, they all start at 1.
There was a problem hiding this comment.
I'm ok to have it set 1 here. The only problem this looks like the last place where assembling with version 0 happens. Probably a separate test checking assembling with version 0 at backwardCompat_test.go is needed
There was a problem hiding this comment.
I don't see a good reason for AssembleStringWithVersion(s, 0) to work at all. There's no "user-level" way to cause the function to run that way. It always runs with the version assemblerNoVersion when invoked to assemble teal outside of tests. I don't think we need to be compatible with old test code. I don't think the WithVersion function should even be public. And it should probably fail if given 0.
data/transactions/logic/assembler.go
Outdated
| @@ -1938,6 +1938,7 @@ type disassembleState struct { | |||
| numericTargets bool | |||
| labelCount int | |||
| pendingLabels map[int]string | |||
| rerun bool | |||
There was a problem hiding this comment.
please add a comment for rerun, it is not obvious at all
jannotti
force-pushed
the
disassemble-backjump
branch
from
cb337cf
to
001848b
Compare
Disassemble was omitting labels if they were only used as targets of backjumps.
This fixes that, but making a second pass to find labels if any such labels exist in a program. It also changes testProg, which is used extensively to test programs in the logic package, so that every such test is a Disassemble/Reassemble test to ensure the same program is produced.