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.

Sign up for GitHub

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

Jump to bottom

Proposal: Displaying Contract IDs in off-ledger DAML #7114

Closed
cocreature opened this issue Aug 13, 2020 · 6 comments · Fixed by #7153
Closed

Proposal: Displaying Contract IDs in off-ledger DAML #7114

cocreature opened this issue Aug 13, 2020 · 6 comments · Fixed by #7153
Assignees
@cocreature
Labels
component/daml-lf DAML-LF discussion Things to be discussed and decided team/ledger-clients Related to the Ledger Clients team's components.
Milestone
Application Runti...

Comments

@cocreature
Copy link
Contributor

Currently, both DAML Script and DAML Triggers are unable to display (i.e., convert to Text) contract IDs. While we do have a Show instance, it always returns the string "<contract-id>" (literally) which is clearly not very useful. This makes debugging significantly harder and we have gotten feedback to that effect quite a few times (it also matches my own experience).

While there are good reasons for not being able to convert a contract id to Text in ledger code this does not hold for off-ledger DAML. In fact, this is something that is trivial to accomplish in all other client bindings. I don’t want to punish our users for trying to use DAML here as well and in fact we recommend it in quite a few instances so I believe we have to address this problem.

Proposal

  • Add a contractIdToText : ContractId -> Optional Text primitive to LF.
  • Add a flag to the speedy Machine class to specify whether we are running on-ledger or off-ledger. In on-ledger mode we always return None. In off-ledger mode we return the contract id as Text.
  • As for the spec, it’s not that hard to add a flag to that as well or alternatively we could completely ignore the off-ledger part since having a spec for that is far less crucial.
  • Implement the Show instance of ContractId in terms of this new primitive keeping the behavior for the on-ledger part to return "<contract-id>".

Alternatives

  • Instead of returning an Optional Text we could return Text and return an empty text or "<contract-id>" or whatever other magic string you come up with. I prefer the Optional since it moves the default used in the Show instance out of the LF-spec but I don’t feel strongly about this.

Alternative Ideas that don’t work

  1. Using a separate ContractId type for DAML Script and DAML Triggers. This seems appealing initially and works for things like exercise and ACS queries. But it falls apart as soon as you have a field of type ContractId in a template or choice argument. Since we reuse those types you now have to interact with both the builtin ContractId type and the one provided by the respective APIs and convert between the two and it just becomes a mess.

  2. Provide a separate contractIdToText primitive as part of DAML Script/DAML Triggers without making it a general LF primitive. This falls into a similar trap as 1. If I call show on a template with a ContractId field I want to see the actual contract id and not have to extract it and then call contractIdToText on it.
@cocreature cocreature added component/daml-lf DAML-LF discussion Things to be discussed and decided labels Aug 13, 2020
@cocreature
Copy link
Contributor Author

cc @hurryabit @remyhaemmerle-da

@hurryabit
Copy link
Contributor

IIRC, one of the reasons why we didn't want a function to convert contract IDs into text was because of the split between local/relative and global/absolute contract IDs and the rewriting that happened after the transaction was built. Since we have removed this rewriting step now and there is only one notion of contract ID, we should put the option of implementing a function to convert contract IDs to text that works the same across all runtimes back on the table. Such a function could clearly be misused but I don't think that should be the only argument stopping us from adding it. Are there any other downsides I'm missing?

@cocreature
Copy link
Contributor Author

My understanding was that ledgers are still free to change contract ids and canton does that.. What they have to do is preserve a prefix or some mapping to the original “relative” contract id. So converting to text doesn’t work.

@remyhaemmerle-da
Copy link
Collaborator

remyhaemmerle-da commented Aug 13, 2020 •
edited
Loading

@cocreature is right, they cannot change contract ids as they want but can add a suffix.
Canton and daml-on-corda use this.

@hurryabit
Copy link
Contributor

Given that ledgers are still allowed to change contract IDs, I think we should go with the original proposal. From the point of view of the DAML-LF specification, which currently only captures the on-ledger use of DAML, the new primitive always returns None. What do you think about using this as the spec of the new primitive until we potentially extend it to also cover the triggers/script use cases?

@cocreature
Copy link
Contributor Author

Sounds reasonable to me. Definitely the simplest option 🙂

@cocreature cocreature self-assigned this Aug 13, 2020
cocreature added a commit that referenced this issue Aug 13, 2020
@cocreature
Add TO_TEXT_CONTRACT_ID primitive
950c1f1 
This is the first part of #7114.

This PR

* Adds the primitive to the protobuf.
* Handles decoding and encoding in Haskell and Scala.
* Handles typechecking in Haskell and Scala.
* Handles speedy compilation and interpretation in Scala.
* Updates the specification.

This PR does not yet change the standard library to make use of this
primitive.

changelog_begin
changelog_end
@cocreature cocreature mentioned this issue Aug 13, 2020
Merged
6 tasks
cocreature added a commit that referenced this issue Aug 17, 2020
@cocreature
Add TO_TEXT_CONTRACT_ID primitive
385f240 
This is the first part of #7114.

This PR

* Adds the primitive to the protobuf.
* Handles decoding and encoding in Haskell and Scala.
* Handles typechecking in Haskell and Scala.
* Handles speedy compilation and interpretation in Scala.
* Updates the specification.

This PR does not yet change the standard library to make use of this
primitive.

changelog_begin
changelog_end
mergify bot pushed a commit that referenced this issue Aug 17, 2020
@cocreature @remyhaemmerle-da
Add TO_TEXT_CONTRACT_ID primitive (#7137)
ab8b418 
* Add TO_TEXT_CONTRACT_ID primitive

This is the first part of #7114.

This PR

* Adds the primitive to the protobuf.
* Handles decoding and encoding in Haskell and Scala.
* Handles typechecking in Haskell and Scala.
* Handles speedy compilation and interpretation in Scala.
* Updates the specification.

This PR does not yet change the standard library to make use of this
primitive.

changelog_begin
changelog_end

* Apply suggestions from code review

Co-authored-by: Remy <remy.haemmerle@daml.com>

* Avoid extra allocation

changelog_begin
changelog_end

Co-authored-by: Remy <remy.haemmerle@daml.com>
cocreature added a commit that referenced this issue Aug 17, 2020
@cocreature
Use TO_TEXT_CONTRACT_ID in Show instance of ContractId
9c6d259 
fixes #7114

This PR changes the Show instance of ContractId and flips the switch
on triggers and DAML Script to run in off-ledger mode.

It also adds a test that for DAML Script we actually get back the
correct contract id.

There is a bit of a design decision here in how we want to print
contract ids, so let me list the options I considered. $cid will stand
for the actual cid and all options are wrapped in markdown inline
code.

1. `"$cid"`. Indistinguishable from string. Suggests that there might
be an IsString instance for ContractId.
2. `<$cid>`. Matches the dummy `<contract-id>` but it’s not a dummy so
I don’t think matching that is benefitial.
3. `$cid`. Easy to spot (contract ids start with # and have no
spaces), clearly not a string but might look slightly weird.

changelog_begin
changelog_end
@cocreature cocreature mentioned this issue Aug 17, 2020
Merged
6 tasks
cocreature added a commit that referenced this issue Aug 17, 2020
@cocreature
Use TO_TEXT_CONTRACT_ID in Show instance of ContractId
c0c1a0c 
fixes #7114

This PR changes the Show instance of ContractId and flips the switch
on triggers and DAML Script to run in off-ledger mode.

It also adds a test that for DAML Script we actually get back the
correct contract id.

There is a bit of a design decision here in how we want to print
contract ids, so let me list the options I considered. $cid will stand
for the actual cid and all options are wrapped in markdown inline
code.

1. `"$cid"`. Indistinguishable from string. Suggests that there might
be an IsString instance for ContractId.
2. `<$cid>`. Matches the dummy `<contract-id>` but it’s not a dummy so
I don’t think matching that is benefitial.
3. `$cid`. Easy to spot (contract ids start with # and have no
spaces), clearly not a string but might look slightly weird.

changelog_begin
changelog_end
cocreature added a commit that referenced this issue Aug 17, 2020
@cocreature
Use TO_TEXT_CONTRACT_ID in Show instance of ContractId
46f41a8 
fixes #7114

This PR changes the Show instance of ContractId and flips the switch
on triggers and DAML Script to run in off-ledger mode.

It also adds a test that for DAML Script we actually get back the
correct contract id.

There is a bit of a design decision here in how we want to print
contract ids, so let me list the options I considered. $cid will stand
for the actual cid and all options are wrapped in markdown inline
code.

1. `"$cid"`. Indistinguishable from string. Suggests that there might
be an IsString instance for ContractId.
2. `<$cid>`. Matches the dummy `<contract-id>` but it’s not a dummy so
I don’t think matching that is benefitial.
3. `$cid`. Easy to spot (contract ids start with # and have no
spaces), clearly not a string but might look slightly weird.

changelog_begin
changelog_end
cocreature added a commit that referenced this issue Aug 17, 2020
@cocreature
Use TO_TEXT_CONTRACT_ID in Show instance of ContractId
cc7f507 
fixes #7114

This PR changes the Show instance of ContractId and flips the switch
on triggers and DAML Script to run in off-ledger mode.

It also adds a test that for DAML Script we actually get back the
correct contract id.

There is a bit of a design decision here in how we want to print
contract ids, so let me list the options I considered. $cid will stand
for the actual cid and all options are wrapped in markdown inline
code.

1. `"$cid"`. Indistinguishable from string. Suggests that there might
be an IsString instance for ContractId.
2. `<$cid>`. Matches the dummy `<contract-id>` but it’s not a dummy so
I don’t think matching that is benefitial.
3. `$cid`. Easy to spot (contract ids start with # and have no
spaces), clearly not a string but might look slightly weird.

changelog_begin
changelog_end
cocreature added a commit that referenced this issue Aug 17, 2020
@cocreature
Use TO_TEXT_CONTRACT_ID in Show instance of ContractId (#7153)
d4d0419 
fixes #7114

This PR changes the Show instance of ContractId and flips the switch
on triggers and DAML Script to run in off-ledger mode.

It also adds a test that for DAML Script we actually get back the
correct contract id.

There is a bit of a design decision here in how we want to print
contract ids, so let me list the options I considered. $cid will stand
for the actual cid and all options are wrapped in markdown inline
code.

1. `"$cid"`. Indistinguishable from string. Suggests that there might
be an IsString instance for ContractId.
2. `<$cid>`. Matches the dummy `<contract-id>` but it’s not a dummy so
I don’t think matching that is benefitial.
3. `$cid`. Easy to spot (contract ids start with # and have no
spaces), clearly not a string but might look slightly weird.

changelog_begin

- [DAML Script/DAML Triggers] When using DAML-LF 1.dev, the `Show` instance of `ContractId` will now display the actual contract id instead of a dummy `<contract-id>` value. Note that this only applies to DAML Script and DAML Triggers not to ledger code.

changelog_end
@stefanobaghino-da stefanobaghino-da added the team/ledger-clients Related to the Ledger Clients team's components. label Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cocreature cocreature

Labels
component/daml-lf DAML-LF discussion Things to be discussed and decided team/ledger-clients Related to the Ledger Clients team's components.
Projects
None yet
Milestone
Application Runtime Backlog
Development

Successfully merging a pull request may close this issue.

Use TO_TEXT_CONTRACT_ID in Show instance of ContractId digital-asset/daml
4 participants
@cocreature @hurryabit @stefanobaghino-da @remyhaemmerle-da