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

plutus-pab: Adds Swagger support for web-server API #3807

Merged
merged 32 commits into from Sep 23, 2021

Conversation

kelizarov
Copy link
Contributor

This change adds a support for swagger using servant-openapi that generates an OpenAPI spec for PAB API with descriptions to provide a non-technical information for how endpoints behave and what they expect in requests and responses.

The swagger-ui is accessible through swagger/swagger-ui/index.html
The example of UI:
telegram-cloud-photo-size-2-5269758382535980541-y

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • 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

@nau nau requested a review from j-mueller August 26, 2021 07:32
Copy link
Contributor

@nau nau left a comment

Choose a reason for hiding this comment

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

You need to run ./updateMaterialized inside nix-shell to update nix files.

@goosedb
Copy link
Contributor

goosedb commented Aug 26, 2021

@nau done!

Copy link
Contributor

@j-mueller j-mueller left a comment

Choose a reason for hiding this comment

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

Making the API self-describing is generally very useful and will make it much easier to generate clients in other languages. We looked at swagger a while ago and the issue that prevented us from using it was lack of support for sum types. But it looks like a new major version of swagger/open API is out so maybe it's time to revisit this.

How does OpenAPI3 handle sum types? For example what does the schema for UniswapContracts look like?

notes/model/UTxO.hsproj/Ledger.hs Outdated Show resolved Hide resolved
@kelizarov
Copy link
Contributor Author

Hey @j-mueller the servant-openapi adds support for Sum types by using oneOf construction. I've fired up Uniswap PAB instance with these changes, here's how the model looks like in swagger-ui:
image

@kelizarov
Copy link
Contributor Author

There's also an issue with representing tuple types in swagger. For example for txRedeemers in Tx type it represented like this:
image

The simple solution could be to declare the ToSchema instance manually instead of using the generic deriviation. Let me know if this is critical here.

@j-mueller
Copy link
Contributor

j-mueller commented Sep 1, 2021

The oneOf construct looks nice, so I'm happy to accept this now.

issue with representing tuple types in swagger

Sorry what is the issue exactly? That the types of the elements of the list aren't specified? In general, encoding a tuple as a heterogeneous list with a fixed length is fine (that is what a tuple is, and that's the default encoding that aeson uses)

@michaelpj
Copy link
Contributor

This PR adds package dependencies to plutus-ledger which are not used, and that's what Hydra complains about: https://github.com/input-output-hk/plutus/pull/3807/files#diff-cb3369701745e982609f4dfe004aa53507b8570ddc9841d1aa4f186109c32725R122-R123

@michaelpj
Copy link
Contributor

I don't think we should add a dependency on openapi to the packages below plutus-ledger-api. Those are depended on by the node, and I don't think we want this in the dependency tree for that. You could put them in your orphans module, perhaps.

shell.nix Outdated Show resolved Hide resolved
@kelizarov
Copy link
Contributor Author

@michaelpj
Copy link
Contributor

No they aren't. You added freer-simple, semigroups, and openapi3, and you're only using openapi3. Which is exactly what the error message said.

@kelizarov
Copy link
Contributor Author

@j-mueller @michaelpj I've added the changes for orphan instances: put them on plutus-ledger package and removed openapi3 dependency from plutus-ledger-api and plutus-core.

@Fiftyw3bs
Copy link

I think the merging of this PR has taken enough time.

I'm stuck at ToSchema not accepting Sum types. This PR seems to be the only block standing in my way ;(

Copy link
Contributor

@j-mueller j-mueller left a comment

Choose a reason for hiding this comment

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

I'd like to get a second opinion from @silky before merging, but it looks good to me now.

plutus-pab/src/Plutus/PAB/Webserver/API.hs Show resolved Hide resolved
Copy link
Contributor

@silky silky left a comment

Choose a reason for hiding this comment

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

Looks great to me! Thanks for doing this work. I've added a couple of small suggestions, then I'm happy to merge!

playground-common/playground-common.cabal Outdated Show resolved Hide resolved
plutus-contract/src/Plutus/Contract/Effects.hs Outdated Show resolved Hide resolved
plutus-contract/src/Wallet/Emulator/Wallet.hs Outdated Show resolved Hide resolved
plutus-ledger/src/Ledger/Orphans.hs Outdated Show resolved Hide resolved
plutus-pab/src/Plutus/PAB/Events/ContractInstanceState.hs Outdated Show resolved Hide resolved
plutus-ledger/src/Ledger/Orphans.hs Outdated Show resolved Hide resolved
@silky
Copy link
Contributor

silky commented Sep 23, 2021

Hey @kelizarov - All looks great; just one final thing ( I promise! sorry :) ) - If you can rebase to master and squish a few of the commits down so the history is a bit cleaner that would be perfect!

@michaelpj
Copy link
Contributor

@silky you can also just do a squash-merge if you like

@silky
Copy link
Contributor

silky commented Sep 23, 2021

Ah thanks @michaelpj ; I looked at 'rebase and merge; only :)
I'll do that!

-- Edit: Thanks @kelizarov for your work on this!

@silky silky merged commit ba62878 into IntersectMBO:master Sep 23, 2021
@michaelpj
Copy link
Contributor

I don't think we should add a dependency on openapi to the packages below plutus-ledger-api. Those are depended on by the node, and I don't think we want this in the dependency tree for that.

This PR still adds a dependency on openapi to plutus-tx, which I said we shouldn't do. @silky can you please move those instances to the orphans module and get rid of the dependency?

@Fiftyw3bs
Copy link

Hi @michaelpj

May I ask how this affects the developers waiting to make use of the solutions in this PR? I, for one, have been stuck for like a week waiting for it to be merged.

@michaelpj
Copy link
Contributor

It shouldn't affect you. I have generously decided not to revert it and instead to let it be fixed in a subsequent PR :)

@silky silky mentioned this pull request Sep 23, 2021
8 tasks
jonesnoaht pushed a commit to jonesnoaht/plutus that referenced this pull request Oct 8, 2021
* Add swagger support via openapi3

Co-authored-by: Danil Berestov <GooseDB@yandex.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants