-
Notifications
You must be signed in to change notification settings - Fork 23
Add AbiMeta struct for CompiledProgram and TemplateProgram
#201
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
base: master
Are you sure you want to change the base?
Conversation
70fc491 to
a9f31ea
Compare
src/serde.rs
Outdated
| let witness_types_converted = self | ||
| .param_types | ||
| .iter() | ||
| .map(|(w_name, w_type)| (w_name.to_string(), w_type.to_string())) |
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.
In a9f31ea:
SimplicityHL might not allow the kinds of perverse constructions that lead to these, but heads up that it's possible in Simplicity to produce types that have small bit-widths but absolutely enormous (like, 2^100+) string serializations. In BlockstreamResearch/rust-simplicity#289 we changed the to_string logic to detect this and to truncate the type serialization if it has more than 10000 nodes, a number which is definitely achievable using "real" SimplicityHL code.
Eventually we will want to define some sort of type encoding (with sharing? unsure). Aside from length limits, this string encoding is currently is full of non-ASCII characters (in particular ×) and extraneous spaces. It's also kinda annoying to parse due to having both infix and postfix operators. Would be better to have a reverse-Polish encoding with no parens.
Anyway we can iterate on this later. No need to hold up this PR on getting the type encoding right.
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.
Also, as a nit, the second witness_types_converted variable ought to have a different name than the first. (Though I find it oddly refreshing to read copy/pasted code rather than LLM-generated code ;))
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.
In a9f31ea:
SimplicityHL might not allow the kinds of perverse constructions that lead to these, but heads up that it's possible in Simplicity to produce types that have small bit-widths but absolutely enormous (like, 2^100+) string serializations. In BlockstreamResearch/rust-simplicity#289 we changed the
to_stringlogic to detect this and to truncate the type serialization if it has more than 10000 nodes, a number which is definitely achievable using "real" SimplicityHL code.Eventually we will want to define some sort of type encoding (with sharing? unsure). Aside from length limits, this string encoding is currently is full of non-ASCII characters (in particular
×) and extraneous spaces. It's also kinda annoying to parse due to having both infix and postfix operators. Would be better to have a reverse-Polish encoding with no parens.Anyway we can iterate on this later. No need to hold up this PR on getting the type encoding right.
Yeah, I understand your concerns.
I tried to implement serialize on that structs WitnessName and ResolvedType, but that was rather impossible without exposing inner struct to pub visibility. It's impossible to extract inner str type hidden under Arc.
It can be done more efficiently but visibility of values has to be increased or we have to implement Deref trait in some way to be able to retrieve reference onto internal str without cloning it.
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.
Yes, I understand that we can't get a str type. We may be able to get a &dyn fmt::Display or something though I'm not sure that serde would handle that properly.
I'm not too worried about going through String here. If this is ever a real performance problem it's definitely possible to fix it by scrapping the HashMaps entirely and manually implementing the serde serialization. But we can do that later.
My concern here is just that the particular strings we're producing are not the ones that we want to use in a production system.
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.
I've implemented serialization with dereferencing. Does it look better or not? In the context of this commit 36166e7.
Serialization without implementing deref can be moved to the type declaration, but serialization is implemented only inside the src/serde.rs file. Is it better to move it or leave it as is?
a9f31ea to
7b0129d
Compare
|
Did you intend to close this? |
|
nope, that was misclick |
|
In 36166e7: Definite concept NACK using We can add an explicit |
chore: * add AsRef impl for some structs for custom serialization
b82c5a3 to
69928ce
Compare
This pr adds possibility to generate
AbiMetastruct both forCompiledProgramandTemplateProgram.This struct is required for generating proper macros that will simplify creation of contracts.
Since retrieving Witness arguments and Parameter types is impossible for
CompiledProgram, I addedAbiMetato improve usability for both compiled and template programs.How it can be used in macros?
From name -> type map we can automatically generate a structs for Witness and Parameters section of contract.
This pattern is consistent across all contracts we have created so far.
https://github.com/BlockstreamResearch/SimplicityHL
By now it is in development, but I don't think that such functionality would negatively impact the code.
Related to these issues #196, #157.
PS
FYI, it is worth noting that functionality to retrieve the AstTreePath for program witnesses could also be added here to derive the proper selection of UTXOs.
https://github.com/ikripaka/SimplicityHL/tree/feature/wallet-abi-meta
However, this PR only adds a subset of those parameters.