adding another field for lsig disassembly

[barnjamin]

Summary

Adding separate field for disassembled lsig program.

closes #3563

Test Plan

None yet

[codecov-commenter]

Codecov Report

Merging #3577 (eb2fe50) into master (bc9cebd) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3577      +/-   ##
==========================================
- Coverage   47.58%   47.56%   -0.03%     
==========================================
  Files         370      370              
  Lines       60092    60093       +1     
==========================================
- Hits        28595    28581      -14     
- Misses      28185    28196      +11     
- Partials     3312     3316       +4     

Impacted Files	Coverage Δ
daemon/algod/api/server/v2/dryrun.go	66.66% <100.00%> (+0.10%)	⬆️
ledger/roundlru.go	90.56% <0.00%> (-5.67%)	⬇️
ledger/blockqueue.go	82.18% <0.00%> (-2.88%)	⬇️
network/wsPeer.go	65.83% <0.00%> (-2.78%)	⬇️
util/metrics/gauge.go	68.00% <0.00%> (-2.67%)	⬇️
util/metrics/counter.go	78.57% <0.00%> (-2.39%)	⬇️
data/abi/abi_type.go	87.67% <0.00%> (-0.95%)	⬇️
network/requestTracker.go	71.12% <0.00%> (+0.86%)	⬆️
crypto/merkletrie/node.go	93.48% <0.00%> (+1.86%)	⬆️
crypto/merkletrie/trie.go	68.61% <0.00%> (+2.18%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc9cebd...eb2fe50. Read the comment docs.

[jannotti]

Would like to get at least one more opinion on this. Current return value certainly seems wrong. It has one place for disassembly, and if there's an app it simply overwrites the logicsig disassembly. I would consider this PR a win compared to that. It adds a new place to tuck the lsig disassembly so that the potential app disassembly overwrites only the first copy of it (which is still returned for backward compatibility).

I wonder if we shouldn't add yet another place for disassembly - a place just for app disassembly. Then, in a latter version we could remove the existing location which is "shared" by logicsigs and apps.

Of course, I don't really see why we're returning disassembly at all. Seems an orthogonal issue to dry-run. But we are, so let's do it well.

Maybe @jasonpaulos @winder @JasonWeathersby would have feelings about such an API change.

[winder]

Would like to get at least one more opinion on this. Current return value certainly seems wrong. It has one place for disassembly, and if there's an app it simply overwrites the logicsig disassembly. I would consider this PR a win compared to that. It adds a new place to tuck the lsig disassembly so that the potential app disassembly overwrites only the first copy of it (which is still returned for backward compatibility).

I wonder if we shouldn't add yet another place for disassembly - a place just for app disassembly. Then, in a latter version we could remove the existing location which is "shared" by logicsigs and apps.

Of course, I don't really see why we're returning disassembly at all. Seems an orthogonal issue to dry-run. But we are, so let's do it well.

Maybe @jasonpaulos @winder @JasonWeathersby would have feelings about such an API change.

The overloaded disassembly field does seem strange. If the field really does have dubious value, a query param to select what (and whether) you'd like to disassemble something would work instead of adding another field.

I'm not familiar with the functionality to have any real opinion about this. Seems fine to add of it's useful, but it does require updating the SDKs.

[jasonpaulos]

I agree that I'm not sure how useful the disassembly field for this endpoint is in general, but the fact that the lsig disassembly gets overridden is a clear bug, so this makes sense as an incremental improvement.

[jannotti]

Looks good. Let's recall that we may wish to add an app specific disassembly field, and then deprecate the old shared field. Perhaps we would do this at once in a breaking v3 update.