Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assembly map #2921

Closed
wants to merge 17 commits into from
Closed

Assembly map #2921

wants to merge 17 commits into from

Conversation

barnjamin
Copy link
Contributor

@barnjamin barnjamin commented Sep 18, 2021

Summary

Adding the concept of an Assembly Map and an option to output a JSON file containing a mapping between TEAL source and assembled bytecode. Addresses #2870

AssemblyMap contains a LineMap and TemplateVariable details

The LineMap provides a more convenient way to identify the TEAL source line that maps to a pc returned in error messages from calls to the algod.

The TemplateLabels provides details about the template variables in the TEAL source including their name, type, and position in the assembled bytecode. This change also allows a template contract to be compiled directly without substituting the TMPL_ prefixed names with temporary "dummy variables".

Test Plan

I tested this by compiling a simple contract with the TMPL_* vars in the source and swapped them out and decompiled to see the values were placed correctly.

Source for the testing I did here:
https://github.com/barnjamin/rareaf/blob/price-tokens/src/contracts/python/tcv.py

There should definitely be some tests in this repo but I'm not sure what you'd like to see.

@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2021

Codecov Report

Merging #2921 (f9a6b90) into master (e466aa1) will decrease coverage by 4.04%.
The diff coverage is 64.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2921      +/-   ##
==========================================
- Coverage   47.40%   43.36%   -4.05%     
==========================================
  Files         369      369              
  Lines       59620    59714      +94     
==========================================
- Hits        28265    25892    -2373     
- Misses      28078    30658    +2580     
+ Partials     3277     3164     -113     
Impacted Files Coverage Δ
cmd/tealdbg/cdtSession.go 38.40% <0.00%> (-29.47%) ⬇️
cmd/goal/clerk.go 8.97% <4.00%> (-0.18%) ⬇️
data/transactions/logic/assembler.go 75.10% <81.08%> (-3.67%) ⬇️
cmd/tealdbg/debugger.go 73.93% <84.21%> (+1.06%) ⬆️
data/committee/sortition/sortition.go 0.00% <0.00%> (-85.72%) ⬇️
debug/logfilter/main.go 0.00% <0.00%> (-83.34%) ⬇️
tools/network/dnssec/client.go 2.94% <0.00%> (-79.42%) ⬇️
ledger/apply/keyreg.go 0.00% <0.00%> (-76.48%) ⬇️
cmd/tealdbg/cdtState.go 10.63% <0.00%> (-73.20%) ⬇️
network/requestLogger.go 0.00% <0.00%> (-72.73%) ⬇️
... and 103 more

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 e466aa1...f9a6b90. Read the comment docs.

@algorandskiy
Copy link
Contributor

I personally do not think source map should be part of logic package. Especially we have it already in tealdbg - see https://github.com/algorand/go-algorand/blob/master/cmd/tealdbg/debugger.go#L256

@barnjamin
Copy link
Contributor Author

Ah, I should have looked elsewhere. I thought it'd be nice to have the map created during assembly since we have everything we need. But if an error returned, a SC author would likely end up running the debug. Maybe a tealdbg subcommand that allows source map generation?

Any opinions about the template stuff?

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.

Is the main idea of the change to allow assembling templates directly? Then assembler replaces TMPL_ with zero and generates a source map that allows external tool to substitute zeros with real values in byte code?
This works perfectly with ints but requires additional efforts for bytes since bytescblock needs to be re-encoded when inserting longer byte arrays.
Is there any benefits to the debugger like showing template variables name while debugging?

In general, source maps proven to be useful so having it in assembler might be ok. In order to make it right I suggest making a separate source map PR with:

  1. The implementation is it is now (logic + goal)
  2. Use implementation from tealdbg as a ground truth (since it is already exposed to users) and generate couple examples
  3. Write a tests that compares new implementation vs examples from tealdbg
  4. Replace tealdbg implementation by a new one.

About templating - I have a concern with bytescblock stuff so asking @JasonWeathersby @jasonpaulos @jannotti for opinion on its usability. You might also consider another approach: extend assembler interface to accept a map of template variables, substitute while assembling and generate var entries in the source map (if there is any benefit of having it). This new func can be called in goal or in algotmpl.

err error
tmpl bool
)
if len(args[0]) > 5 && args[0][:5] == TmplPrefix {
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above. This actually can be handled in ops.handleTemplateVar(args[0]) that would check the prefix and populate ops.TemplateLabels. And it can be used in all three functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the template labels require some position/type information, I've not implemented this yet though it may still be a good option

@barnjamin
Copy link
Contributor Author

Is the main idea of the change to allow assembling templates directly? Then assembler replaces TMPL_ with zero and generates a source map that allows external tool to substitute zeros with real values in byte code?

Essentially yes. The main idea is to allow the resultant bytecode of a template contract to have known template variable positions. The benefit of that is that it needs to be compiled only once and variables (properly encoded) can be injected directly into the bytecode without an additional compile step.

This can also be used for what I've been calling "Template Contract Validation", where for example a stateful sc can validate that the account opting in is an instance of the template contract through some bytecode manipulation && comparing the hash to a known hash of the blank contract.

I like the idea of having a goal command for doing the variable insertion but I think it would also make sense to provide the functionality in the SDKs since improper encoding can be dangerous.

I'll leave this open just to get any feedback but start on another PR implementing what you described.

@jasonpaulos
Copy link
Contributor

jasonpaulos commented Sep 22, 2021

I like the idea of having a goal command for doing the variable insertion but I think it would also make sense to provide the functionality in the SDKs since improper encoding can be dangerous.

I just got here and haven't looked at any code yet, but I fully agree with this. Templated contracts are frequently used to create LogicSigs, and the process is pretty complicated today. It would help a lot if goal had a special compile mode for templated contracts, and if the SDKs could use the output of this compilation to provide values for template variables.

@barnjamin
Copy link
Contributor Author

#2975 serves to provide more info on the pc => line mapping

Would still like to have a deeper discussion around template contracts but closing this pr in favor of the above

@barnjamin barnjamin closed this Oct 4, 2021
@barnjamin
Copy link
Contributor Author

Reopening but not ready for review yet

@barnjamin barnjamin reopened this Dec 3, 2021
@barnjamin barnjamin marked this pull request as draft December 3, 2021 11:09
@barnjamin barnjamin closed this Dec 16, 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