-
Notifications
You must be signed in to change notification settings - Fork 205
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
Provide contract choice metadata #15116
Conversation
CHANGELOG_BEGIN CHANGELOG_END
@@ -44,7 +46,8 @@ protected ContractCompanion( | |||
String templateClassName, | |||
Identifier templateId, | |||
Function<String, Id> newContractId, | |||
Function<DamlRecord, Data> fromValue) { | |||
Function<DamlRecord, Data> fromValue, | |||
List<ChoiceMetadata<Data, ?, ?>> choices) { |
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.
Here's what I suggest we do with this thing:
- add a
Marker
type parameter toContractTypeCompanion
. ForInterfaceCompanion
it should beI
, forContractCompanion
it should beData
- Pass
choices
along tosuper
(substitutingMarker
forData
in the type) - transform it to the instance variable
public final Map<String, ChoiceMetadata<Marker, ?, ?>> choices
(seeContractDecoder
for an example that makes it safe and immutable)
That would be reasonable for this PR; for interfaces we can either include them in this PR or do a separate PR to supply them.
/** The commonality between {@link ContractCompanion} and {@link InterfaceCompanion}. */ | ||
public abstract class ContractTypeCompanion { | ||
/** The full template ID of the template or interface that defined this companion. */ | ||
public final Identifier TEMPLATE_ID; | ||
//TODO: Need to figure out the right type params here | ||
private final List<ChoiceMetadata<?, ?, ?>> choices; |
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.
@S11001001 I was about to ask about this and saw your comment from yesterday. I think adding that Marker
type will solve my problem
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.
We're going to change it again in #15125, so might as well get started now.
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.
With #14037 I was thinking "well I don't have a use for this type parameter yet on ContractTypeCompanion
, so I shouldn't add it until we have a use for it".
But I think in retrospect this was the wrong decision, and compatibility considerations actually make it better to preemptively type-parameterize such newly-introduced types to our public API, if we have in mind a use for them later.
The reasoning for the extra tparam in #15125 is a lot less obvious, so I doubt I would have correctly anticipated the need for that one, but certainly having Marker
already would have been wise.
Of course this change in thinking is why ChoiceMetadata
has its third tparam; we don't use it in this PR, but I'm fairly sure we have a good use for it, so we should just have it so as to make compatibility better.
# Conflicts: # language-support/java/bindings/src/main/java/com/daml/ledger/javaapi/data/codegen/ContractTypeCompanion.java # language-support/java/bindings/src/main/java/com/daml/ledger/javaapi/data/codegen/InterfaceCompanion.java # language-support/java/codegen/src/main/scala/com/digitalasset/daml/lf/codegen/backend/java/inner/ContractIdClass.scala # language-support/java/codegen/src/main/scala/com/digitalasset/daml/lf/codegen/backend/java/inner/InterfaceClass.scala # language-support/java/codegen/src/main/scala/com/digitalasset/daml/lf/codegen/backend/java/inner/TemplateClass.scala
# Conflicts: # language-support/java/codegen/src/main/scala/com/digitalasset/daml/lf/codegen/backend/java/inner/TemplateClass.scala
interfaceName, | ||
packagePrefixes, | ||
interface.choices, | ||
withPrefixes = false, // TODO: remove in #15154 |
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.
Removing this in the next ticket
@@ -5,18 +5,22 @@ | |||
|
|||
import com.daml.ledger.javaapi.data.Identifier; | |||
|
|||
import java.util.List; | |||
|
|||
/** The commonality between {@link ContractCompanion} and {@link InterfaceCompanion}. */ | |||
public abstract class ContractTypeCompanion<Maker, Data> { |
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.
Not clear what this is, let's discuss with @S11001001
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.
Please rename Maker
to Marker
as well, this was a typo.
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.
@ray-roestenburg-da ContractCompanion
represents templates, InterfaceCompanion
interfaces, and the union of the two...
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.
(from other discussion) For an interface, the type argument to ContractId
(Marker
) is different from the payload type i.e. interface view (Data
); for a template they are the same.
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.
Run ./fmt.sh --diff
in the root to fix the Java format.
Additionally, sample CHOICE_*
fields should be documented in docs/source/app-dev/bindings-java/codegen.rst
- near the definition of
COMPANION
for templates - near the definition of field
INTERFACE
for interfaces
The details of the initializer can be elided; what matters is that it is clear that such fields are defined, what their types are, and users can refer to Javadoc for further information.
@@ -5,18 +5,22 @@ | |||
|
|||
import com.daml.ledger.javaapi.data.Identifier; | |||
|
|||
import java.util.List; | |||
|
|||
/** The commonality between {@link ContractCompanion} and {@link InterfaceCompanion}. */ | |||
public abstract class ContractTypeCompanion<Maker, Data> { |
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.
Please rename Maker
to Marker
as well, this was a typo.
.../java/bindings/src/main/java/com/daml/ledger/javaapi/data/codegen/ContractTypeCompanion.java
Outdated
Show resolved
Hide resolved
@@ -5,18 +5,22 @@ | |||
|
|||
import com.daml.ledger.javaapi.data.Identifier; | |||
|
|||
import java.util.List; | |||
|
|||
/** The commonality between {@link ContractCompanion} and {@link InterfaceCompanion}. */ | |||
public abstract class ContractTypeCompanion<Maker, Data> { |
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.
@ray-roestenburg-da ContractCompanion
represents templates, InterfaceCompanion
interfaces, and the union of the two...
...support/java/bindings/src/main/java/com/daml/ledger/javaapi/data/codegen/ChoiceMetadata.java
Outdated
Show resolved
Hide resolved
...support/java/bindings/src/main/java/com/daml/ledger/javaapi/data/codegen/ChoiceMetadata.java
Outdated
Show resolved
Hide resolved
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.
Additionally,
- test that you can use a
CHOICE_*
field to retrieve the name of a choice in codegen output - test that the
choices
maps' keysets in aCOMPANION
and anINTERFACE
correspond to expected sets of choice names
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.
Docs portion looks good.
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.
Very nice. 👍 Feel free to merge after addressing these comments.
.../src/test/scala/com/digitalasset/daml/lf/codegen/backend/java/ChoiceMetadataFieldsSpec.scala
Outdated
Show resolved
Hide resolved
.../src/test/scala/com/digitalasset/daml/lf/codegen/backend/java/ChoiceMetadataFieldsSpec.scala
Outdated
Show resolved
Hide resolved
CHANGELOG_BEGIN - Add new representation of a choice, ChoiceMetadata. - Generated templates and interfaces now include fields for each available choice. These fields will have a ChoiceMetadata type and will be called CHOICE_N, where N represents the choices' name. CHANGELOG_END
Fixes #14329.
CHANGELOG_BEGIN
CHANGELOG_END