Skip to content
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

Investigate app-runtime changes to allow templates to implement interfaces with clashing-name choices #13668

Closed
6 tasks done
stefanobaghino-da opened this issue Apr 22, 2022 · 12 comments
Assignees
Labels
component/java-ecosystem Java development experience component/js-ecosystem TypeScript and React.js bindings component/json-api HTTP JSON API discussion Things to be discussed and decided roadmap/interfaces https://digitalasset.atlassian.net/browse/DAML-56 team/ledger-clients Related to the Ledger Clients team's components.

Comments

@stefanobaghino-da
Copy link
Contributor

stefanobaghino-da commented Apr 22, 2022

Referring to #13653, lifting this requirement will likely have a knock-on effect on runtime components where we have interface support already. The outcome of this ticket should be:

  • an analysis of how lifting the requirement will impact each affected runtime component
  • a suggestion of how to change each affected runtime component to deal with the lifted requirement
  • a clear statement of whether we expect some breaking change with regards to stable features (breaking changes with regards to interface support is expected, of course)

Possibly affected runtime components:

Unlikely to be affected but included for completeness:

@stefanobaghino-da stefanobaghino-da added component/java-ecosystem Java development experience component/json-api HTTP JSON API component/js-ecosystem TypeScript and React.js bindings team/ledger-clients Related to the Ledger Clients team's components. labels Apr 22, 2022
@S11001001 S11001001 changed the title Investigation: evaluate changes necessary to runtime components as we allow templates to implement interfaces that define choices with clashing names Investigate app-runtime changes to allow templates to implement interfaces with clashing-name choices Apr 22, 2022
@S11001001 S11001001 added the discussion Things to be discussed and decided label Apr 22, 2022
@S11001001
Copy link
Contributor

S11001001 commented Apr 22, 2022

Java codegen

Impact

Without changes, each overloaded choice name will get codegenned support for one of the choices, with the remainder silently discarded. Additionally, users will get invalid ExerciseCommands for interface choices invoked from template-contract-IDs.

Java codegen takes the LF interface library as input; as the main structural invariant is lifted from the interface library, i.e. each template maps each ChoiceName to exactly one choice signature, its template and interface generators will stop compiling and need fixes.

Suggested changes

Remove inherited exercise methods entirely.

Where exercise-by-cid methods are currently generated directly in T.ContractId, generate them instead in a separate interface:

  public interface Exercises<Cmd> extends com.daml.ledger.javaapi.data.codegen.Exercises<Cmd> {
    default Cmd exerciseTestTemplate_Int(TestTemplate_Int arg) {
      return makeExerciseCmd(arg.toValue());
    }
    default Cmd exerciseTestTemplate_Int(Long x) {
      return exerciseTestTemplate_Int(new TestTemplate_Int(x));
    }
    // and another pair for each choice
  }

codegen.Exercises will supply the makeExerciseCmd signature, and codegen.ContractId will implement this signature for Exercises<ExerciseCommand>.

For ContractId classes, mix in these exercises, and replace the interface-cid conversion:

  public static final class ContractId extends codegen.ContractId<Tpl> implements Exercises<ExerciseCommand> {
    // remove current `to*` and `unsafeFrom*` methods
    // for every implemented interface, generate
    public pkg.Ifc.ContractId toInterface(pkg.Ifc.INTERFACE marker) { /* implementation elided */ }
    public static ContractId unsafeFromInterface(pkg.Ifc.ContractId cid) { /* ... */ }
  }

We use toInterface overloads because it lets us rely on Java's own name conflict resolution instead of defining our own. If you define multiple interfaces with the same name, this marker will be present in separate packages for each, and you can handle this with Java's own import mechanism.

createAndExercise redesign

Deprecate every current createAndExercise method, and remove the interface-inherited ones entirely. Instead, generate a pair of a new class and a method:

  public static final class createAnd extends codegen.CreateAnd<...> {
    createAnd(...) {super(...)}
    public pkg.Ifc.createAnd toInterface(pkg.Ifc.INTERFACE marker) {
      return new pkg.Ifc.createAnd(...);
    }
    // one for each inherited interface
  }
  public createAnd createAnd() {return new createAnd(...);}

Supply the needed makeExerciseCmd by implementing Exercises<CreateAndExerciseCommand> in codegen.CreateAnd.

interface definition changes

In interfaces themselves, we also produce interface Exercises<Cmd> and class createAnd in exactly the same way as with templates, with the exception that the createAnd constructor is public, and there are no interface-conversion methods for the latter. Likewise we mix in Exercises<ExerciseCommand> into the interface ContractId class in exactly the same way.

We need an additional class, though, the marker class. This can serve a similar role for interfaces that COMPANION does for templates, as we can put whatever metadata we want to be conveniently abstract in it. However, in this design, it only absolutely has to be there for overload signature distinction with the toInterface and unsafeFromInterface methods.

  public static final class INTERFACE extends codegen.InterfaceCompanion<...> {
    INTERFACE() {/*...*/} // deliberately package-private
  }
  public static final INTERFACE INTERFACE = new INTERFACE();

We need a separate class because that is the only way to get the toInterface overloads to have separate erasures.

Will break

This removes the new toIfc and unsafeFromIfc methods, as well as interface methods inherited directly for exercise on template; any calls using these recently introduced, interface-only features will stop working. Strict code using create-and-exercise will run into deprecation warnings that will point them to the new create-and-exercise pattern.

@S11001001
Copy link
Contributor

S11001001 commented Apr 22, 2022

LF Interface library

Impact

The interface library, in contrast with the LF decoded AST, encodes invariants about the serializable signatures in an LF archive as tightly as possible, so the codegens and JSON API have sound basis inputs. The InterfaceReader needs to loosen these restrictions, especially where the "one choice per name" might be implicitly assumed, and the data structures and functions dealing with choices will all need to change to account for more-than-one.

As it will no longer be possible to invoke interface choices via the template ID on the ledger API, the representation of choice maps also needs to indicate when an interface ID needs to be passed to exercise.

Suggested changes

In retrospect, managing DefTemplate state dynamically purely by emptiness of the unresolvedInheritedChoices is more brittle than I desired. Instead, it (but not DefInterface) should contain a member ADT with two cases:

// 1
    choices: Map[Ref.ChoiceName, TemplateChoice[Ty]],
    unresolvedInheritedChoices: NonEmpty[Map[Ref.ChoiceName, Ref.TypeConName]],
// 2
    choices: Map[Ref.ChoiceName,
                 NonEmpty[Map[Option[Ref.Identifier], TemplateChoice[Ty]]]],

Surprisingly, it is not necessary to change the map key from ChoiceName. The parameter type in the TemplateChoice is sufficient to identify the nominal choice.

We will remove the option to not failIfUnresolvedChoicesLeft and always fail in this case.

We could still do this but for now the "drop unresolved choices on the floor" option still exists.

Will break

Any code that uses template signatures from the interface library. But this is what we want.

@S11001001
Copy link
Contributor

S11001001 commented Apr 26, 2022

Typescript codegen

Impact

Generated Template TypeScript-interfaces expect to be able to key each Choice uniquely, and use TypeScript extends to incorporate the interfaces that represent interfaces. Like so:

interface SomeInterfaceInterface<T> {
  Split: Choice<...>
  Another: Choice<...>
}
const SomeInterface: Template<...> & SomeInterfaceInterface<object>

interface ActualTemplateInterface extends SomeInterfaceInterface<ActualTemplate> {
  Split: Choice<...>
  Mine: Choice<...>
}
const ActualTemplate: Template<...> & ActualTemplateInterface

As this is supposed to represent two incompatible Split choices, with different template IDs even, this extends relationship can no longer be valid for every template and implemented interface.

Suggested changes

Extend @daml/types with a function alongside registerTemplate that replaces the generated Object.assign invocations in renderTemplateDef, that will perform the interface-companion/template-companion combination described below, rather than the trivial one currently done there.

Rather than unconditionally copying the interface Choices to the template companion, do not do so if the choice name occurs in either another interface or in the template. This means that Split would not be copied in the above example.

When generating the extends list above, for any *Interface parents with a choice that was excluded according to the above rule, remove the excluded choices with Omit. So for example, in place of SomeInterfaceInterface<ActualTemplateInterface>, extend Omit<SomeInterfaceInterface<ActualTemplateInterface>, "Split">, passing the union of excluded choice names as the second argument.

Currently and under this design, these are well-typed with respect to atContractId and choicePayload:

myLedger.exercise(ActualTemplate.Mine, atContractId, choicePayload)
myLedger.exercise(ActualTemplate.Another, atContractId, choicePayload)
myLedger.exercise(ActualTemplate.Split, atContractId, choicePayload)

However, this is not well-typed with respect to atContractId, though it still is with respect to choicePayload:

myLedger.exercise(SomeInterface.Split, atContractId, choicePayload)

We will recommend that users invoke the inherited choice (a la Another above) where that is possible, but not fix the lack of typing in the final example in this change.

While JSON API can resolve ambiguities, for simplicity, codegen-driven exercises by contract ID and key will pass both the templateId and interfaceChoiceId unconditionally to JSON API, even in case of template-defined choices.

Will break

This will not break anything for existing TypeScript codegen users on existing Daml code.

@S11001001
Copy link
Contributor

S11001001 commented Apr 26, 2022

JSON API

Impact

JSON API assumes that you can pass a template ID for a gRPC exercise of an interface choice; this will no longer be allowed by gRPC, but we should keep supporting it for JSON API's own argument interface.

There will be new error cases for when a choice resolution is ambiguous, similar to when package ID inference fails.

Suggested changes

A choice resolution additionally produces an optional interface ID; this is reflected in the interface library. When an interface ID is yielded, JSON API must exercise by interface ID; otherwise, it must exercise by template ID.

For its own exercise endpoint, an optional choiceInterfaceId may be specified, whose default is the templateId. (An interface ID will continue to be allowed as a templateId argument, subject to the restrictions below.) The semantics depend on where the choices come from and what type of ID choiceInterfaceId is.

  1. If a template ID is specified, and the template directly defines the choice, that is the selected choice; inherited choices are ignored.
  2. If an interface ID is specified, the choice must come from the interface. This exactly matches current behavior.
  3. If a template ID is specified, and the template does not define the choice, but inherits choices with that name, what happens depends on how many:
    1. If only one, that choice is selected.
    2. If more than one, HTTP 400 indicating the interface IDs on which the choice is defined.

Note that it will continue to be required that templateId is not an interface ID in the exercise-by-key case; to exercise an ambiguous choice on an interface by key, users will specify the key's template type as templateId and the choice's interface as choiceInterfaceId.

create-and-exercise is an open question in #13653 at time of writing, so I have left that part TODO.

Will break

This will not break anything for existing JSON API users on existing Daml code.

@S11001001
Copy link
Contributor

Trigger service

Impact

The trigger runner's Converter may have to parse different commands; this doesn't imply any ambiguity for that conversion, though, as the exact defining interface-or-template is always statically known when compiling a trigger.

None of this complexity propagates upward to the trigger service.

Suggested changes

Incorporate the new trigger runner.

Will break

Triggers that were written against the older trigger library may break and need to be recompiled; the new triggers will require the newly-compiled trigger service as well. There are no trigger-service-specific breaks.

@stefanobaghino-da
Copy link
Contributor Author

We don't have to commit to a solution right away, I'll make sure to think more about this next week. For the time being let's re-prioritize Scala codegen work. In that area we have more freedom to make changes, as long as they don't break existing code. 🕷️

@S11001001
Copy link
Contributor

S11001001 commented May 13, 2022

Scala codegen

Impact

Codegen won't break in a way that is immediately apparent, but will have no way to generate the correct exercise method when confronted with overloaded names. It will also currently fail if an interface-contract-ID appears in any signature, even though interface-contract-ID types are serializable.

Naturally, it must also adapt to changes in the LF interface library described above.

Suggested changes

For every interface If, codegen an uninhabited marker type

sealed abstract class If extends Interface[If]
object If extends InterfaceCompanion[If] {
  override val id = // ...
  implicit final class `If syntax`[+` ExOn`](private val id : ` ExOn`) extends AnyVal {
    // exercise methods exactly like with templates
  }
}

Common utilities can be factored between the existing TemplateCompanion and the new InterfaceCompanion. Note that If is uninhabited, so several members of TemplateCompanion will not make sense.

exercise will need a new argument, the optional interface ID. Generated template exercise methods will not include this, and generated interface exercise methods will include it unconditionally.

Add to object Template a class Implements[T, If]. This shouldn't have any usage but as a marker, but there's no problem adding data to this class if it turns out to be useful.

Add an implicit class to object Template for ContractId[T] that adds this method:

def toInterface[If](implicit ev: Implements[T, If]): ContractId[If]

and likewise an unsafeToTemplate to object Interface to provide the opposite conversion. This will completely avoid the same-named-template problem with these methods, because the way you select the "right" interface is to simply use Scala's own name management.

Template.CreateForExercise and Template.Key also get toInterface with the same signature. unsafeToTemplate is possible, but I cannot imagine a use case; these are strictly transient types.

In each generated template companion, add one implicit val for each interface If of type Template.Implements[T, If]. The name should contain a space, and can contain the fully-qualified interface name to avoid collisions; there is no need to ever import it or refer to it explicitly.

// exercise
cid.toInterface[If].exerciseFoo(party, args...)
// exercise by key
Tpl.key((alice, 42)).toInterface[If].exerciseFoo(party, args...)
// create-and-exercise
Tpl(alice, 1, 2).createAnd.toInterface[If].exerciseFoo(party, args...)

// coerce interface-cid to template-cid
icid.unsafeToTemplate[Tpl].exerciseBar(party, args)

I am fairly certain we can elide the toInterface calls by means of implicits. However, this should be an optional feature, if provided at all; it could be activated by means of an import.

Will break

This will undo the inheritance added as a workaround in #13858; all existing calls to interface exercise methods will no longer compile. As this feature has no external users, and possibly no internal users at time of writing, this is a minimal impact.

@stefanobaghino-da
Copy link
Contributor Author

stefanobaghino-da commented May 17, 2022

There has been progress with regards to the Java codegen. I'll report here the outcome of the conversation I had with @S11001001.

The idea is the following

  • for every interface there will be a static singleton representing that interface
  • for every interface implemented by a template, there will be a toInterface method defined on the template contract ID taking as an input a specific singleton and returning the interface contract ID expected by that conversion

This boils down to the following (the names are there only to illustrate the concept and are not to be considered final):

templateContractId.toInterface(com.daml.Asset.INTERFACE).exerciseSomeChoice(arguments);
templateContractId.toInterface(com.example.Asset.INTERFACE).exerciseSomeChoice(arguments);

This will work for edge cases as the one above in which there is a clash in unqualified names, choice names and arguments at the same time.

Note that if you already have an interface contract ID you can invoke the choice method directly, making the experience uniform and especially well suited to focus your work around interfaces as we make them more prominent through Daml.

As part of the conversation, we also mentioned the following to allow to issue a create-and-exercise like so:

templateContractId.createAnd(arguments).toInterface(com.daml.Asset.INTERFACE).exerciseSomeChoice(arguments);

The new method would also be available to used to run createAnd(...).exerciseSomeChoice(arg) directly on the template.

In order to make the experience smooth, we would also mark the createAndExercise methods as deprecated. This will allow to reduce the surface of the generated code in the future while retaining full functionality.

Regardless, the approach is a breaking change with regards to the current status of Java codegen for interfaces, which is fine since we're still in the early access stage and breaking changes are allowed before we move on to GA.

@S11001001
Copy link
Contributor

templateContractId.createAnd(arguments)

with the minor caveat

payload.createAnd()

@S11001001
Copy link
Contributor

S11001001 commented May 17, 2022

@stefanobaghino-da I'm updating the detailed design, too, but to summarize we can split Java codegen into 3 blocks of work:

  1. Remove .toIfc() and .unsafeFromIfc(...) methods and introduce the singletons and overloads, which should be a named final class with package-private ctor that extends a class from the codegen package.
  2. Deprecate the createAndExercise methods and introduce two-phase. This will entail also generating the exercise methods separately from contract ID class, having them use an inherited helper method instead of new ExerciseCommand directly, and mixing them into contract ID class instead.
  3. Remove inherited exercise methods and deal with the changed structures from daml-lf/interface library. This is the only change then needed or depending on overloaded interface choices.

@S11001001
Copy link
Contributor

@stefanobaghino-da I've rewritten Java codegen to explain how to implement the new design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/java-ecosystem Java development experience component/js-ecosystem TypeScript and React.js bindings component/json-api HTTP JSON API discussion Things to be discussed and decided roadmap/interfaces https://digitalasset.atlassian.net/browse/DAML-56 team/ledger-clients Related to the Ledger Clients team's components.
Projects
None yet
Development

No branches or pull requests

2 participants