-
Notifications
You must be signed in to change notification settings - Fork 35
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
My take on dialects, which I think is now uniform in interface with the run_program from stage_0 #98
base: main
Are you sure you want to change the base?
Conversation
…he run_program from stage_0
clvm/chia_dialect.py
Outdated
try: | ||
cost, result = prev(value) | ||
except TypeError: | ||
cost, result = prev(value,max_cost) |
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.
Can you run black
on all the .py
files to standardize formatting?
pip install black
black clvm tests
KEYWORD_FROM_ATOM = {int_to_bytes(k): v for k, v in enumerate(KEYWORDS)} | ||
KEYWORD_TO_ATOM = {v: k for k, v in KEYWORD_FROM_ATOM.items()} | ||
|
||
KEYWORD_TO_LONG_KEYWORD = { |
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.
Can you build this table algorithmically from OP_REWRITE
?
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.
Or vice-versa?
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'm not sure this constant is actually Chia dialect-specific.
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 included it there mainly to avoid another layer when resolving dependencies, but it does make sense to put this on a layer in between.
@@ -7,6 +7,7 @@ | |||
|
|||
dependencies = [ | |||
"blspy>=0.9", | |||
"clvm_rs>=0.1.8" |
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 think it's best to make clvm_rs
an optional dependency. I expect that clvm_rs
will eventually be completely standalone.
CLVMAtom = Any | ||
CLVMPair = Any | ||
|
||
CLVMObjectType = Union["CLVMAtom", "CLVMPair"] |
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 know I originally wrote this file, but I'm wondering if maybe we should remove CLVMObjectType
since it's not actually used anywhere (and maybe other unused things in this file). It doesn't seem that mypy's typing system is advanced enough to do what we need to do.
@@ -1,4 +1,4 @@ | |||
brun --backend=python -c -v '(x)' | |||
brun -c -v '(x)' |
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.
Whoops these should be replaced with a dialect choice. Will fix that.
May be controversial.
I made cuts needed to move some constants into a place where importing them would be possible from anywhere, improving dependency congestion.
Main visible differences:
As used in clvm_tools, it's now possible to
The dialect object is factored out of global paths, so it at least owns all its own information.