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

Extracting the contents of the `agreement` field of a contract instance through the Sandbox API #1110

Closed
gyorgybalazsi opened this issue May 13, 2019 · 14 comments

Comments

@gyorgybalazsi
Copy link

commented May 13, 2019

The agreement field of templates and contracts is what makes a DAML contract really useful in legal settings.

Would be nice to have the option to extract the contents of the agreement field through the Sandbox API and the Navigator UI.

@gerolf-da

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

While adding a new field agreementText to the protobuf CreatedEvent is trivial, adding the field to the data types in the java bindings would be a breaking change. In particular:

  • data.CreatedEvent in the Java Bindings
    • new field of type String, requiring an additional constructor parameter => BREAKING CHANGE
  • the Decoder class generated by the Java Codegen
    • change decoder from BiFunction<String, Record, Contract> to Function<CreatedEvent, Contract> => BREAKING CHANGE
  • the SomeTemplate.Contract class generated by the Java Codegen
    • change static method fromIdAndRecord(String, Record) to fromCreatedEvent(CreatedEvent) => BREAKING CHANGE

We could make the change backwards compatible by:

  • making the Contract#agreementText field Optional[String] instead of String.
  • keeping and deprecating the existing methods mentioned above, always setting the agreement text to None (even if in fact there is an agreement text) or alternatively just an empty string, if we stick with the type String

Looking at Template.daml, the default agreement is an empty String. We could treat this as the None case for the Optional?

agreement : c -> Text
agreement _ = ""

I'd like to avoid that we set the agreement text of a contract to None or an empty string, simply because the user was using a deprecated method (i.e. fromIdAndRecord instead of fromCreatedEvent). Or is it enough to highlight this in the documentation?

@bitonic

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

@gerolf-da I think the changes need not to be breaking if we keep everything optional, as you say.

However, we cannot just stick an empty string in: we must distinguish the case where we do not have the agreement text because the Ledger API didn't give it to us (or in any case because "we don't have it"), and the case where the agreement text is empty.

@gerolf-da

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

the case where we do not have the agreement text because the Ledger API didn't give it to us (or in any case because "we don't have it")

@bitonic: This would only be the case when using newer bindings with an older sandbox. I guess we want to cover this use case?

@gerolf-da

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

Although, from a client binding perspective, we cannot distinguish between an empty string and "agreement" not set (because protobuf string is a primitive), unless we use something like the StringValue wrapper for the agreementText field.

@bitonic

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

@gerolf-da that's solved by adding

bool hasAgreement = X;
bool agreement = string;

rather than just agreement.

@gerolf-da

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

Meh, that's super ugly. Also, hasAgreement will get accessor methods like isHasAgreement. I'd rather prefer the StringWrapper in that case or a dedicated message message OptionalAgreement { string agreement = 1; }

@bitonic

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

@gerolf-da the boolean + field is a "pattern" of sorts in protobuf. i think it is the most idiomatic way of doing that. the wrapper is quite a bit more awkward, imo.

@gerolf-da

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

We haven't yet employed that pattern in the Ledger API though. I guess there's always a first at some point :)

@bitonic

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

@gerolf-da as far as I can tell we have never added a required string to the ledger API. so I don't think this pattern was ever relevant, unless I'm misunderstanding.

@gerolf-da

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

The point is to make it not required / optional to support the use case of newer client + older sandbox.

I've quickly run through the change locally, and I think this pattern is worse than the StringValue wrapper. Why? Because the separate flag is easily overlooked, whereas accessing a field of a message type means it must be checked for presence first. Granted, this can also be "forgotten", but I think it's more clear from the type of the field alone.

@bitonic

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

@gerolf-da yes, my wording was poor.

Feel free to go ahead with the wrapper, but I'd call it AgreementText rather than StringValue. I'd wait for a few more instances before establishing this as a pattern and giving it a generic name.

@gerolf-da

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

StringValue would come from the google's wrappers.proto. But I don't mind using a dedicated message for this.

@bitonic

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

@gerolf-da sorry, I completely missed that Google provides this wrapper already. In that case, using that seems like the sensible thing to do. Sorry about the back and forth. I should have clicked your previous link.

gerolf-da added a commit that referenced this issue May 15, 2019

gerolf-da added a commit that referenced this issue May 15, 2019

@gerolf-da

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

My current proposal on how the agreement is displayed in Navigator:
image

gerolf-da added a commit that referenced this issue May 15, 2019

gerolf-da added a commit that referenced this issue May 16, 2019

gerolf-da added a commit that referenced this issue May 16, 2019

gerolf-da added a commit that referenced this issue May 17, 2019

gerolf-da added a commit that referenced this issue May 17, 2019

Expose a contract's agreement text on the Ledger API (#1151)
* Added agreement_text field to the CreatedEvent in Ledger API.
* Changed java bindings + java codegen
* Changed utilities for scala codegen
* Made necessary changes in Sandbox to propagate the agreement text from ContractInst to the CreatedEvent
* Made changes to the navigator to show the agreement text in the contract details page when it is set and not empty

Fixes #1110
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.