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

SCP-1879: Export serialised PLC programs with envelopes #3358

Merged
merged 1 commit into from
Jun 16, 2021
Merged

SCP-1879: Export serialised PLC programs with envelopes #3358

merged 1 commit into from
Jun 16, 2021

Conversation

koslambrou
Copy link
Contributor

@koslambrou koslambrou commented Jun 12, 2021 •
edited
Loading

Add the ability to export serialised PLC programs with Cardano API envelopes.

Pre-submit checklist:

  • Branch
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Relevant tickets are mentioned in commit messages
    • Formatting, materialized Nix files, PNG optimization, etc. are updated
  • PR
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

Pre-merge checklist:

  • Someone approved it
  • Commits have useful messages
  • Review clarifications made it into the code
  • History is moderately tidy; or going to squash-merge

@koslambrou
Copy link
Contributor Author

@michaelpj Is this what you had in mind?

@koslambrou koslambrou changed the title Export serialised PLC programs with envelopes SCP-1879: Export serialised PLC programs with envelopes Jun 12, 2021
@koslambrou koslambrou force-pushed the scp-1879-plc-serialize-envelopes branch from 073aad7 to 415ed4d Compare June 12, 2021 17:57
@michaelpj
Copy link
Contributor

Yep, perfect. Two things to think about:

  1. Can we set it up so that people importing Plutus.Ledger.Scripts also pull in these instances? So we can try to pretend they're not orphans.
  2. Can you update https://github.com/input-output-hk/plutus/blob/master/doc/plutus/howtos/exporting-a-script.rst to show how to do this? It's pretty dumb, just showing that you can call the function is sufficient, I think.

@koslambrou
Copy link
Contributor Author

Exporting the instances in Plutus.V1.Ledger.Scripts with import Ledger.TextEnvelope () introduces a cyclic dependency. Ledger.TextEnvelope relies on the Script datatype of Plutus.V1.Ledger.Scripts.

@michaelpj
Copy link
Contributor

Well, we certainly can't put it there since plutus-ledger-api doesn't depend on cardano-api (and shouldn't). But we ought to be able to do it in plutus-ledger, I'd have thought?

@koslambrou koslambrou force-pushed the scp-1879-plc-serialize-envelopes branch from 0e7e053 to f60d7e5 Compare June 14, 2021 12:46
@koslambrou koslambrou requested a review from michaelpj June 14, 2021 12:47
@koslambrou koslambrou marked this pull request as ready for review June 14, 2021 12:47
@koslambrou koslambrou force-pushed the scp-1879-plc-serialize-envelopes branch from f60d7e5 to 65acd85 Compare June 14, 2021 15:46
michaelpj
michaelpj reviewed Jun 15, 2021
View reviewed changes
Copy link
Contributor

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, sorry, forgot to actually click submit review...

doc/plutus/howtos/exporting-a-script.rst Outdated
You can translate directly into CBOR using the `Serialise` typeclass from the `serialise` package.
Or you can create an envelope of the sort used by the Cardano node CLI like the following:

.. code:: haskell
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, what the directive below does is to include a chunk of an actual Haskell file that we compile. That contains the example. So could you add this example to that file, which will then make sure it keeps compiling! Basically we don't use ..code if we can help it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for that. Didn't know how it worked.

Is that good?

plutus-ledger/plutus-ledger.cabal
@@ -71,7 +72,6 @@ library
Plutus.V1.Ledger.DCert as Ledger.DCert,
Plutus.V1.Ledger.Crypto as Ledger.Crypto,
Plutus.V1.Ledger.Interval as Ledger.Interval,
Plutus.V1.Ledger.Scripts as Ledger.Scripts,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this makes sense. Maybe put a line comment here explaining that we manually re-do that module so we can include some extra instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That good?

@koslambrou
Copy link
Contributor Author

BTW, just noticed that there's a Validator wrapper type around Script. Should the HasTextEnvelope instance be with Validator instead of Script?

@michaelpj
Copy link
Contributor

BTW, just noticed that there's a Validator wrapper type around Script. Should the HasTextEnvelope instance be with Validator instead of Script?

Good question! No, because some scripts are also minting policies. However, it does raise the question of whether we should have different envelope types for validators and minting policies...

For now, just leave it as it is and let's think about it.

@koslambrou koslambrou force-pushed the scp-1879-plc-serialize-envelopes branch 3 times, most recently from aca315c to 62faa76 Compare June 15, 2021 13:53
@koslambrou koslambrou force-pushed the scp-1879-plc-serialize-envelopes branch from 62faa76 to c3ce3f2 Compare June 15, 2021 16:15
@koslambrou koslambrou requested a review from michaelpj June 15, 2021 17:16
@michaelpj michaelpj merged commit 3bc9731 into master Jun 16, 2021
@koslambrou koslambrou deleted the scp-1879-plc-serialize-envelopes branch June 16, 2021 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelpj michaelpj michaelpj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@koslambrou @michaelpj