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

Modify submit-api to accept cbor transactions instead #2491

Merged

Conversation

newhoggy
Copy link
Contributor

No description provided.

@newhoggy newhoggy force-pushed the modify-submit-api-to-accept-cbor-transactions-instead branch from 67bacb5 to 1610818 Compare March 16, 2021 03:47
Copy link
Contributor

@erikd erikd left a comment

Choose a reason for hiding this comment

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

Instead? Instead of what? It makes sense to accept CBOR, but removing something else to replace it with CBOR does not make too much sense.

@newhoggy
Copy link
Contributor Author

This comment is the context: #2370 (comment)

My understanding is that CBOR without envelope is what the old version of cardano-submit-api did, so the change is to revert to that behaviour for backwards compatibility.

We would probably do some MIME type thing to support both, I haven't figured that out yet.

@newhoggy newhoggy force-pushed the modify-submit-api-to-accept-cbor-transactions-instead branch from 1610818 to 870e643 Compare March 16, 2021 07:15
cardano-submit-api/src/Cardano/TxSubmit/Web.hs Outdated Show resolved Hide resolved
@newhoggy newhoggy force-pushed the modify-submit-api-to-accept-cbor-transactions-instead branch from 870e643 to ef2968d Compare March 17, 2021 06:06
@newhoggy newhoggy requested a review from dcoutts March 17, 2021 07:23
@newhoggy newhoggy force-pushed the modify-submit-api-to-accept-cbor-transactions-instead branch 2 times, most recently from 9ba8d31 to fc0afd7 Compare March 17, 2021 07:26
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM! I linked to a commit with a suggestion (up to you if you want to use it). Just squash the commits and you can merge 👍

]
-> Either RawCborDecodeError b
deserialiseAnyOf ts te = let (es, as) = partitionEithers results in maybe (errors es) Right (listToMaybe as)
where
Copy link
Contributor

@Jimbo4350 Jimbo4350 Mar 17, 2021

Choose a reason for hiding this comment

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

An alternative suggestion: 862f692

Copy link
Contributor Author

@newhoggy newhoggy Mar 17, 2021

Choose a reason for hiding this comment

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

I've taken your suggestion to use list instead of non-empty-list, but also fixed up the logic like this: If there is at least successful decode, always take the first one.

@newhoggy newhoggy force-pushed the modify-submit-api-to-accept-cbor-transactions-instead branch from b52b91c to 06ca656 Compare March 17, 2021 13:30
@newhoggy newhoggy dismissed dcoutts’s stale review March 17, 2021 13:31

Comments addressed

@newhoggy
Copy link
Contributor Author

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Mar 18, 2021

@iohk-bors iohk-bors bot merged commit a16adc7 into master Mar 18, 2021
@iohk-bors iohk-bors bot deleted the modify-submit-api-to-accept-cbor-transactions-instead branch March 18, 2021 01:17
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

4 participants