Add Boolean.toByte operation #932
Conversation
| @@ -507,6 +507,10 @@ trait GraphBuilding extends SigmaLibrary { IR: IRContext => | |||
| else | |||
| error(s"The type of $obj is expected to be Collection to select 'size' property", obj.sourceContext.toOption) | |||
|
|
|||
| case Select(obj, SBoolean.ToByte, _) if obj.tpe.isBoolean => | |||
| val bool = eval(obj.asBoolValue) | |||
There was a problem hiding this comment.
Not sure if this can be done inline like this or maybe we need to add a class in transformers and a new (or existing?) OpCode for boolean.toByte
There was a problem hiding this comment.
I guess opcode is not needed, from investigating MethodCallSerializer
There was a problem hiding this comment.
yes, opcode is not necessary. Another question here is how to make sure VersionContext.current has correct version in place when this code is executed as part of the application/tooling compiling ErgoTree?
ross-weir
force-pushed
the
bool-to-byte-op
branch
from
aaaa68a
to
7e45336
Compare
| @@ -202,6 +203,21 @@ class TestingInterpreterSpecification extends CompilerTestingCommons | |||
| |}""".stripMargin) | |||
| } | |||
|
|
|||
| property("Evaluate boolean casting ops") { | |||
There was a problem hiding this comment.
This property runs the test block for each activated script version / ergotree version. Something is causing SigmaParser to execute a different code path between test cases, SBoolean.getMethods is only called at the start when version is v0 so it returns Seq() for methods, presumably even for v3 since getMethods isn't called when the version is set to v3 and the test fails with a "Missing method" error.
I'm not sure if this is intended and methods are cached somewhere or if there's a global var not getting reset between test cases, or something else. Need to debug deeper
There was a problem hiding this comment.
There's caching indeed, in types.scala: 370
lazy val methods: Seq[SMethod] = {
changing lazy val to def is fixing tests.
I think methods should be reworked in a way that for every version methods should be cached, and then def methods should return value from cache for VersionContext.current (or calculate value and cache it).
There was a problem hiding this comment.
yes, the cache should be versioned, the suggested change sounds good.
Basically the cache needs to change from Seq[SMethod] to a function Version => Seq[SMethod], which can be implemented with the help of Map[Byte, Seq[SMethod]] under the hood.
| _currErgoTreeVersion.withValue(treeVersion)(block) | ||
| _currErgoTreeVersion.withValue(treeVersion) { | ||
| VersionContext.withVersions(activatedVersion, treeVersion)(block) | ||
| } |
There was a problem hiding this comment.
What is the reason for this change?
| @@ -507,6 +507,10 @@ trait GraphBuilding extends SigmaLibrary { IR: IRContext => | |||
| else | |||
| error(s"The type of $obj is expected to be Collection to select 'size' property", obj.sourceContext.toOption) | |||
|
|
|||
| case Select(obj, SBoolean.ToByte, _) if obj.tpe.isBoolean => | |||
| val bool = eval(obj.asBoolValue) | |||
There was a problem hiding this comment.
yes, opcode is not necessary. Another question here is how to make sure VersionContext.current has correct version in place when this code is executed as part of the application/tooling compiling ErgoTree?
| .withInfo(PropertyCall, "Convert true to 1 and false to 0") | ||
|
|
||
| protected override def getMethods(): Seq[SMethod] = { | ||
| if (VersionContext.current.isEvolutionActivated) { |
There was a problem hiding this comment.
This code influences the MethodCall deserialization as part of ErgoTree deserialization. And we need to make a decision here. If we just enable this based only on activatedVersion, then is will be available after the activation on mainnet for all ErgoTrees regardless of their version. Is this what we want?
Should ErgoTree v0 be allowed to have new methods?
There was a problem hiding this comment.
It should not, but I do not see where tree-based version can be injected with the current design, in both serializers and here tree version is not available. So we need to discuss where to inject tree version.
There was a problem hiding this comment.
It is more or less clear how to make activatedVersion and ergoTreeVersion available in this code place when it is used in the interpreter, I can do this. Because the values are coming from the block being validated.
But this code is also used in compiler, and there we need to decide, how applications should respect those versions. I.e. if some dApp or tool uses the compiler, how the versions can be specified?
There was a problem hiding this comment.
I think SigmaByteReader should include tree version. Interpreter is another issue.
if some dApp or tool uses the compiler, how the versions can be specified
this question already exists in the wild, as compiler sets ErgoTree version.
| @@ -202,6 +203,21 @@ class TestingInterpreterSpecification extends CompilerTestingCommons | |||
| |}""".stripMargin) | |||
| } | |||
|
|
|||
| property("Evaluate boolean casting ops") { | |||
There was a problem hiding this comment.
yes, the cache should be versioned, the suggested change sounds good.
Basically the cache needs to change from Seq[SMethod] to a function Version => Seq[SMethod], which can be implemented with the help of Map[Byte, Seq[SMethod]] under the hood.
sc/shared/src/test/scala/sigmastate/TestingInterpreterSpecification.scala
Outdated
Show resolved
Hide resolved
ross-weir
force-pushed
the
bool-to-byte-op
branch
from
12e2a3e
to
0f76431
Compare
Let me know if anything more needs to be done (tests, etc)
closes #931