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

Lenient SealedTx decoding #2946

Merged
merged 6 commits into from
Oct 5, 2021
Merged

Conversation

ghost
Copy link

@ghost ghost commented Oct 4, 2021

This is my very first PR in cardano-wallet so my apologies if I broke some rules of the land :)

This simple PR allows clients to provide either a base16 or base64 encoding when submitting a transaction for signing and other endpoints relevant for the new signing process. It is based off of @KtorZ's #2943 PR as this strand of work was triggered by us tripping ourselves in the foot trying to sign a base16 encoded transaction.

@ghost ghost self-assigned this Oct 4, 2021
@ghost ghost requested review from paweljakubas and KtorZ October 4, 2021 15:35
@ghost
Copy link
Author

ghost commented Oct 4, 2021

I guess the PR fails because of the commits it is based on? Will try to rebase on master

@KtorZ KtorZ force-pushed the abailly-iohk/lenient-sealedtx-decoding branch from 1328e62 to 2a068d8 Compare October 4, 2021 17:36
@KtorZ
Copy link
Member

KtorZ commented Oct 4, 2021

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 4, 2021

👎 Rejected by too few approved reviews

forAll selectFromPreparedBinaries $ \ bs ->
let result = parseJSONSealedTx $ Aeson.encode $ ApiBytesT @'Base64 bs
in result == Right bs &
counterexample ("Parse result: " <> show result)
Copy link
Member

Choose a reason for hiding this comment

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

👍 convincing properties.

parseJSON = fmap ApiT . parseSealedTxBytes @'Base64
parseJSON v = do
tx <- parseSealedTxBytes @'Base16 v <|> parseSealedTxBytes @'Base64 v
pure $ ApiT tx
Copy link
Member

Choose a reason for hiding this comment

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

👍

@ghost ghost marked this pull request as draft October 5, 2021 06:05
@ghost
Copy link
Author

ghost commented Oct 5, 2021

I did not run all unit tests before pushing and some of them are failing. Will revert to draft status, apologies for the inconvenience.

Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

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

lgtm. The properties will be even better when selectFromPreparedBinaries is replaced with genTx from cardano-api:gen soon (@sevanspowell is working on this)

@paweljakubas paweljakubas marked this pull request as ready for review October 5, 2021 06:58
@paweljakubas
Copy link
Contributor

bors r+

iohk-bors bot added a commit that referenced this pull request Oct 5, 2021
2946: Lenient SealedTx decoding r=paweljakubas a=abailly-iohk

> This is my very first PR in cardano-wallet so my apologies if I broke some rules of the land :)

This simple PR allows clients to provide either a base16 or base64 encoding when submitting a transaction for signing and other endpoints relevant for the new signing process. It is based off of @KtorZ's #2943 PR as this strand of work was triggered by us tripping ourselves in the foot trying to sign a base16 encoded transaction.

Co-authored-by: Arnaud Bailly <[email protected]>
Co-authored-by: Pawel Jakubas <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 5, 2021

Build failed:

@ghost
Copy link
Author

ghost commented Oct 5, 2021

Need to fix a unit test, will push soon...

@paweljakubas paweljakubas force-pushed the abailly-iohk/lenient-sealedtx-decoding branch from cbc6ae0 to f1b28ae Compare October 5, 2021 08:32
@paweljakubas
Copy link
Contributor

bors r+

iohk-bors bot added a commit that referenced this pull request Oct 5, 2021
2946: Lenient SealedTx decoding r=paweljakubas a=abailly-iohk

> This is my very first PR in cardano-wallet so my apologies if I broke some rules of the land :)

This simple PR allows clients to provide either a base16 or base64 encoding when submitting a transaction for signing and other endpoints relevant for the new signing process. It is based off of @KtorZ's #2943 PR as this strand of work was triggered by us tripping ourselves in the foot trying to sign a base16 encoded transaction.

Co-authored-by: Arnaud Bailly <[email protected]>
Co-authored-by: Pawel Jakubas <[email protected]>
@paweljakubas
Copy link
Contributor

bors r-

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 5, 2021

Canceled.

@paweljakubas
Copy link
Contributor

bors r+

iohk-bors bot added a commit that referenced this pull request Oct 5, 2021
2946: Lenient SealedTx decoding r=paweljakubas a=abailly-iohk

> This is my very first PR in cardano-wallet so my apologies if I broke some rules of the land :)

This simple PR allows clients to provide either a base16 or base64 encoding when submitting a transaction for signing and other endpoints relevant for the new signing process. It is based off of @KtorZ's #2943 PR as this strand of work was triggered by us tripping ourselves in the foot trying to sign a base16 encoded transaction.

Co-authored-by: Arnaud Bailly <[email protected]>
Co-authored-by: Pawel Jakubas <[email protected]>
@ghost
Copy link
Author

ghost commented Oct 5, 2021

Sorry for the confusion, PR now complete with test fixed and documentation added in swagger.yaml.

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 5, 2021

Build failed:

@paweljakubas paweljakubas force-pushed the abailly-iohk/lenient-sealedtx-decoding branch from 018dadf to 6e3d825 Compare October 5, 2021 11:43
@paweljakubas
Copy link
Contributor

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 5, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 7f04c4f into master Oct 5, 2021
@iohk-bors iohk-bors bot deleted the abailly-iohk/lenient-sealedtx-decoding branch October 5, 2021 13:42
WilliamKingNoel-Bot pushed a commit that referenced this pull request Oct 5, 2021
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.

3 participants