feat(external-call): Add proto definitions and data types#438
feat(external-call): Add proto definitions and data types#438trusch wants to merge 1 commit intodigital-asset:mainfrom
Conversation
|
🎉 Thank you for your contribution! It appears you have not yet signed the Agreement DA Contributor License Agreement (CLA), which is required for your changes to be incorporated into an Open Source Software (OSS) project. Please kindly read the and reply on a new comment with the following text to agree: I have hereby read the Digital Asset CLA and agree to its terms You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
9417a28 to
526e9eb
Compare
526e9eb to
34dd477
Compare
paulbrauner-da
left a comment
There was a problem hiding this comment.
Overall looks good but please address comments.
| string config_hash = 3; // Hash of the extension configuration for version validation | ||
| string input_hex = 4; // Hex-encoded input data sent to the extension | ||
| string output_hex = 5; // Hex-encoded output data returned by the extension |
There was a problem hiding this comment.
config_hash, input_hex, and output_hex should have type bytes, not string. The daml language doesn't support strings, that's why we use an hex encoding in LF. (We may introduce proper support for bytes in LF one day.) But in the transaction, it is a waste of space to represent them as strings.
| string config_hash = 3; // Hash of the extension configuration for version validation | ||
| string input_hex = 4; // Hex-encoded input data sent to the extension | ||
| string output_hex = 5; // Hex-encoded output data returned by the extension | ||
| int32 call_index = 6; // Index of this call within the exercise node (for ordering) |
There was a problem hiding this comment.
This not needed because repeated is ordered.
| extensionId: String, | ||
| functionId: String, | ||
| configHash: String, | ||
| inputHex: String, | ||
| outputHex: String, | ||
| callIndex: Int, |
There was a problem hiding this comment.
Same comment as in the protobuf definition: configHash, inputHex and outputHex should be daml.lf.data.Bytes. callIndex is not necessary.
|
|
||
| //SECP256K1_VALIDATE_KEY = 4001; // REMAINS HERE TEMPORARELY TO PARSE CHECKED IN DARS | ||
|
|
||
| EXTERNAL_CALL = 5001; // *Available in versions >= 2.dev* |
There was a problem hiding this comment.
LF (the language that the daml compiler produces and that the engine consumes) is separate from the language of the transactions produced by the engine, even if they tend to refer to them as "LF transactions". For that reason, we tend to introduce LF changes and transaction changes in separate PRs. I suggest you split this PR into a transaction one and an LF one.
When you modify LF you also need to:
- Modify LF syntax test parser in
com.digitalasset.daml.lf.testing.parserand its spec. - Add a trivial test for typing in
TypingSpec(which uses the parser above) - Add it to
Builtin_2.dev_.lf
…l calls - Add EXTERNAL_CALL builtin to daml_lf2.proto - Add ExternalCallResult to transaction.proto - Extend Node.scala with externalCallResults field - Update Ast.scala with BEExternalCall - Add language feature flag
34dd477 to
9c23c57
Compare
Summary
This PR adds the foundational proto definitions and Scala data types for the external call feature. It defines how external call results are represented in transactions and the language AST.
Changes
Proto Definitions
daml_lf2.prototransaction.protoScala Data Types
Node.scalaexternalCallResults: Seq[ExternalCallResult]toExercisenodeExternalCallResultcase classAst.scalaBEExternalCalltoBuiltinFunctionsealed traitLanguageFeatureGen.scalaTransaction.scalaSerializationVersion.scalaValidation
Typing.scalaBEExternalCallbuiltinDecoding
DecodeV2.scalaBUILTIN_EXTERNAL_CALLTesting
TransactionSpec.scala- Transaction with external callsValueGenerators.scala- Generators for external call results