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

Better rendering of bech32 error messages #1625

Merged
merged 1 commit into from
May 7, 2020
Merged

Better rendering of bech32 error messages #1625

merged 1 commit into from
May 7, 2020

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented May 5, 2020

Like #1619, but with master as base branch 🤦‍♂️

@Anviking Anviking self-assigned this May 5, 2020
@Anviking Anviking requested a review from KtorZ May 5, 2020 15:33
KtorZ
KtorZ approved these changes May 6, 2020
View reviewed changes
Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

bors r+

@KtorZ
Copy link
Member

KtorZ commented May 6, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request May 6, 2020
@iohk-bors @Anviking
Merge #1625
7a0e570 
1625: Better rendering of bech32 error messages r=KtorZ a=Anviking

Like #1619, but with `master` as base branch 🤦‍♂️ 


Co-authored-by: Johannes Lund <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 6, 2020

Timed out

@Anviking
Copy link
Member Author

Anviking commented May 6, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request May 6, 2020
@iohk-bors @Anviking
Merge #1625
423a7a7 
1625: Better rendering of bech32 error messages r=Anviking a=Anviking

Like #1619, but with `master` as base branch 🤦‍♂️ 


Co-authored-by: Johannes Lund <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 6, 2020

Build failed

@Anviking
Copy link
Member Author

Anviking commented May 6, 2020 

test/integration/Test/Integration/Byron/Scenario/CLI/Transactions.hs:563:56:
  1) CLI Specifications, BYRON_TXS_CLI, CLI_TRANS_ESTIMATE_01/02 - 2 recipient(s)
       uncaught exception: RequestException
       DecodeFailure "Error in $: parsing [] failed, expected Array, but encountered Object: Response {responseStatus = Status {statusCode = 404, statusMessage = \"Not Found\"}, responseVersion = HTTP/1.1, responseHeaders = [(\"Transfer-Encoding\",\"chunked\"),(\"Date\",\"Wed, 06 May 2020 08:11:29 GMT\"),(\"Server\",\"Warp/3.3.5\")], responseBody = \"{\\\"code\\\":\\\"no_such_wallet\\\",\\\"message\\\":\\\"I couldn't find a wallet with the given id: da15d905f705267a54b8f1fc74251fb3fc720cc5\\\"}\", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}"

  To rerun use: --match "/CLI Specifications/BYRON_TXS_CLI/CLI_TRANS_ESTIMATE_01/02 - 2 recipient(s)/"

  test/integration/Test/Integration/Byron/Scenario/CLI/Transactions.hs:713:31:
  2) CLI Specifications, BYRON_TXS_CLI, CLI_TRANS_ESTIMATE_04 - Error shown when ErrInputsDepleted encountered
       uncaught exception: RequestException
       DecodeFailure "Error in $: parsing [] failed, expected Array, but encountered Object: Response {responseStatus = Status {statusCode = 404, statusMessage = \"Not Found\"}, responseVersion = HTTP/1.1, responseHeaders = [(\"Transfer-Encoding\",\"chunked\"),(\"Date\",\"Wed, 06 May 2020 08:11:32 GMT\"),(\"Server\",\"Warp/3.3.5\")], responseBody = \"{\\\"code\\\":\\\"no_such_wallet\\\",\\\"message\\\":\\\"I couldn't find a wallet with the given id: da15d905f705267a54b8f1fc74251fb3fc720cc5\\\"}\", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}"

  To rerun use: --match "/CLI Specifications/BYRON_TXS_CLI/CLI_TRANS_ESTIMATE_04 - Error shown when ErrInputsDepleted encountered/"

  test/integration/Test/Integration/Byron/Scenario/CLI/Transactions.hs:736:31:
  3) CLI Specifications, BYRON_TXS_CLI, CLI_TRANS_ESTIMATE_04 - Can't cover fee
       uncaught exception: RequestException
       DecodeFailure "Error in $: parsing [] failed, expected Array, but encountered Object: Response {responseStatus = Status {statusCode = 404, statusMessage = \"Not Found\"}, responseVersion = HTTP/1.1, responseHeaders = [(\"Transfer-Encoding\",\"chunked\"),(\"Date\",\"Wed, 06 May 2020 08:11:38 GMT\"),(\"Server\",\"Warp/3.3.5\")], responseBody = \"{\\\"code\\\":\\\"no_such_wallet\\\",\\\"message\\\":\\\"I couldn't find a wallet with the given id: da15d905f705267a54b8f1fc74251fb3fc720cc5\\\"}\", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}"

  To rerun use: --match "/CLI Specifications/BYRON_TXS_CLI/CLI_TRANS_ESTIMATE_04 - Can't cover fee/"

bors r+

iohk-bors bot added a commit that referenced this pull request May 6, 2020
@iohk-bors @Anviking
Merge #1625
95c2be5 
1625: Better rendering of bech32 error messages r=Anviking a=Anviking

Like #1619, but with `master` as base branch 🤦‍♂️ 


Co-authored-by: Johannes Lund <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 6, 2020

Build failed

@KtorZ KtorZ added the IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG label May 6, 2020
@rvl
Copy link
Contributor

rvl commented May 7, 2020

Those test failures look like a problem with flaky test setup. It says the wallet doesn't exist.
The failures are on macOS by the way.

rvl
rvl reviewed May 7, 2020
View reviewed changes
lib/cli/src/Cardano/CLI.hs
where
go _c [] [] = mempty
go c (i:is) (s:ss)
| c == i = red ++ s:def ++ go (c + 1) is ss
Copy link
Contributor

Choose a reason for hiding this comment

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

This will output 2 ansi escape sequences for each invalid character.

Copy link
Member Author

@Anviking Anviking May 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, but it still works, and multiple invalid characters should be rare.

@Anviking
Better bech32 error messages
1f36bc5 
- marking invalid characters red

$ cardano-wallet-byron key public xprv18rp[..]
Bech32 error: Invalid character(s) in string
xprv18rppm7hzl6[...]qvy6zgldkö
@Anviking Anviking force-pushed the anviking/ADP-195/render-bech32-errors branch from 7b53de7 to 1f36bc5 Compare May 7, 2020 11:00
@Anviking
Copy link
Member Author

Anviking commented May 7, 2020

did rebasing help?
bors r+

Anviking
Anviking commented May 7, 2020
View reviewed changes
lib/cli/src/Cardano/CLI.hs
unCharPos (CharPosition x) = x

markCharsRedAtIndices :: Integral i => [i] -> String -> String
markCharsRedAtIndices ixs txt = go 0 ixs txt ++ def
Copy link
Member Author

Choose a reason for hiding this comment

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

Should perhaps sort the ixs here 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Think it's off, will follow up with property test and fix

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 7, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit 617a4d7 into master May 7, 2020
@iohk-bors iohk-bors bot deleted the anviking/ADP-195/render-bech32-errors branch May 7, 2020 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvl rvl rvl left review comments

@KtorZ KtorZ KtorZ approved these changes

Assignees

@Anviking Anviking

Labels
IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG
Projects
None yet
Milestone
(ADP-190, ADP-195) Public key derivat...
Development

Successfully merging this pull request may close these issues.
3 participants
@Anviking @KtorZ @rvl