-
Notifications
You must be signed in to change notification settings - Fork 493
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
Assembly map #2921
Conversation
… out the map json file
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
I personally do not think source map should be part of |
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? |
There was a problem hiding this 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:
- The implementation is it is now (logic + goal)
- Use implementation from tealdbg as a ground truth (since it is already exposed to users) and generate couple examples
- Write a tests that compares new implementation vs examples from tealdbg
- 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.
data/transactions/logic/assembler.go
Outdated
err error | ||
tmpl bool | ||
) | ||
if len(args[0]) > 5 && args[0][:5] == TmplPrefix { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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 I'll leave this open just to get any feedback but start on another PR implementing what you described. |
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 |
#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 |
Reopening but not ready for review yet |
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.