Extend deserialiseFromRawBytesHex to produce error description #3304

cblp · 2021-10-20T14:42:58Z

No description provided.

Jimbo4350

Right, let's reorganize these commits:
Commit 1: Change to deserialiseFromRawBytesHex
Commit 2: Propagating this change throughout the repo
Commit 3: Miscellaneous refactorings

cblp · 2021-12-23T15:24:53Z

Right, let's reorganize these commits: Commit 1: Change to deserialiseFromRawBytesHex Commit 2: Propagating this change throughout the repo Commit 3: Miscellaneous refactorings

done

Jimbo4350

Right direction, a few comments

Jimbo4350 · 2021-12-31T15:36:33Z

cardano-api/src/Cardano/Api/SerialiseRaw.hs

-    case Base16.decode hex of
-      Right raw -> deserialiseFromRawBytes proxy raw
-      Left _msg -> Nothing
+                           => AsType a -> ByteString -> Either String a


String isn't a particularly indicative error type. We should create a custom datatype, for example:

data RawBytesHexError = RawBytesHexErrorBase16DecodeFail ByteString String | RawBytesHexErrorRawBytesDecodeFail ByteString String

We can also implement an Error instance to render the errors.

I think it should be done in another patch. String is used already everywhere.

String is used already everywhere.

And it shouldn't be. Even more reason to change the error type.

cardano-api/src/Cardano/Api/Utils.hs

cardano-cli/src/Cardano/CLI/Helpers.hs

cblp · 2022-03-24T11:06:35Z

bors merge

3304: Extend deserialiseFromRawBytesHex to produce error description r=cblp a=cblp Co-authored-by: Yuriy Syrovetskiy <yuriy.syrovetskiy@iohk.io>

iohk-bors · 2022-03-24T11:36:40Z

Build failed:

ci/hydra-build:required

cblp · 2022-04-22T17:24:04Z

bors merge

3304: Extend deserialiseFromRawBytesHex to produce error description r=cblp a=cblp Co-authored-by: Yuriy Syrovetskiy <yuriy.syrovetskiy@iohk.io>

iohk-bors · 2022-04-22T17:55:40Z

This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried.

Additional information:

{"message":"Waiting on code owner review from MarcFontaine, cleverca22, deepfire, denisshevchenko, and/or jutaro.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

cblp · 2022-04-27T09:53:25Z

bors merge

3304: Extend deserialiseFromRawBytesHex to produce error description r=cblp a=cblp Co-authored-by: Yuriy Syrovetskiy <yuriy.syrovetskiy@iohk.io>

iohk-bors · 2022-04-27T10:25:15Z

This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried.

Additional information:

{"message":"Waiting on code owner review from MarcFontaine, cleverca22, deepfire, denisshevchenko, and/or jutaro.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

dcoutts · 2022-04-28T15:50:27Z

cardano-api/src/Cardano/Api/SerialiseRaw.hs

+data RawBytesHexError
+  = RawBytesHexErrorBase16DecodeFail
+      ByteString -- ^ original input
+      String -- ^ error message


Can we not do better than a String here?

Sorry? Too much indirection for my English.

Do you suggest having something more structured than a String in the second field of RawBytesHexErrorBase16DecodeFail or not?

cardano-api/src/Cardano/Api/Script.hs

dcoutts · 2022-04-28T15:56:24Z

cardano-api/src/Cardano/Api/TxBody.hs

-  case deserialiseFromRawBytesHex AsTxId (BSC.pack str) of
-    Just addr -> return addr
-    Nothing -> fail $ "Incorrect transaction id format:: " ++ show str
+  str <- some Parsec.hexDigit <?> "transaction id (hexadecimal)"


Was there a reason to switch many1 to some? The Parsec version has an implementation specialised to parsers.

some is a less surprising name for the same thing.

dcoutts · 2022-04-28T16:00:03Z

cardano-cli/src/Cardano/CLI/Shelley/Parsers.hs

@@ -1974,7 +1975,7 @@ parseTxIn = TxIn <$> parseTxId <*> (Parsec.char '#' *> parseTxIx)

 parseTxId :: Parsec.Parser TxId
 parseTxId = do
-  str <- Parsec.many1 Parsec.hexDigit Parsec.<?> "transaction id (hexadecimal)"
+  str <- some Parsec.hexDigit <?> "transaction id (hexadecimal)"


Same question. Is this change needed? Why?

MarcFontaine

LGTM concerning the tx-generator

cblp · 2022-05-02T15:27:21Z

bors merge

iohk-bors · 2022-05-02T16:16:32Z

Build succeeded:

cblp requested review from DavidEichmann, dcoutts, erikd, JaredCorduan, Jimbo4350, nc6 and newhoggy as code owners October 20, 2021 14:42

cblp mentioned this pull request Oct 20, 2021

Serialize AssetName to JSON as hex string #3211

Merged

cblp force-pushed the cblp/deserialiseFromRawBytesHex branch 2 times, most recently from 3d6754f to 49395c0 Compare October 21, 2021 16:23

cblp requested review from deepfire, denisshevchenko, jutaro and MarcFontaine as code owners October 21, 2021 16:23

cblp force-pushed the cblp/deserialiseFromRawBytesHex branch from 49395c0 to bbf878a Compare October 27, 2021 15:47

cblp force-pushed the cblp/deserialiseFromRawBytesHex branch from bbf878a to c6b9946 Compare November 19, 2021 13:41

cblp force-pushed the cblp/deserialiseFromRawBytesHex branch from c6b9946 to 2f8a9a7 Compare December 3, 2021 15:57

Jimbo4350 requested changes Dec 7, 2021

View reviewed changes

cblp force-pushed the cblp/deserialiseFromRawBytesHex branch from 2f8a9a7 to 0c449a8 Compare December 23, 2021 15:24

cblp requested a review from Jimbo4350 December 23, 2021 15:24

cblp force-pushed the cblp/deserialiseFromRawBytesHex branch from 0c449a8 to 602084e Compare December 24, 2021 14:18

Jimbo4350 requested changes Dec 31, 2021

View reviewed changes

cblp force-pushed the cblp/deserialiseFromRawBytesHex branch 2 times, most recently from 5d6bc07 to 039952d Compare January 11, 2022 17:04

cblp requested a review from Jimbo4350 January 11, 2022 17:05

cblp force-pushed the cblp/deserialiseFromRawBytesHex branch 4 times, most recently from 994a333 to 6e9ecac Compare January 21, 2022 13:56

cblp requested a review from cleverca22 as a code owner February 17, 2022 01:26

cblp force-pushed the cblp/deserialiseFromRawBytesHex branch from 3911a12 to 8ce6b0f Compare March 14, 2022 16:59

cblp added this to Review in cardano-node/cardano-api/cardano-cli Mar 14, 2022

cblp force-pushed the cblp/deserialiseFromRawBytesHex branch from 8ce6b0f to 5dacedb Compare March 23, 2022 16:52

iohk-bors bot added a commit that referenced this pull request Mar 24, 2022

Merge #3304

6f9a580

3304: Extend deserialiseFromRawBytesHex to produce error description r=cblp a=cblp Co-authored-by: Yuriy Syrovetskiy <yuriy.syrovetskiy@iohk.io>

cblp force-pushed the cblp/deserialiseFromRawBytesHex branch 2 times, most recently from 64dd0ec to 7c4093e Compare March 25, 2022 16:22

cblp force-pushed the cblp/deserialiseFromRawBytesHex branch 2 times, most recently from 2337440 to f8835a0 Compare April 15, 2022 15:05

cblp force-pushed the cblp/deserialiseFromRawBytesHex branch 2 times, most recently from 94c2835 to 87645b3 Compare April 20, 2022 15:43

iohk-bors bot added a commit that referenced this pull request Apr 22, 2022

Merge #3304

f43cebe

3304: Extend deserialiseFromRawBytesHex to produce error description r=cblp a=cblp Co-authored-by: Yuriy Syrovetskiy <yuriy.syrovetskiy@iohk.io>

iohk-bors bot added a commit that referenced this pull request Apr 27, 2022

Merge #3304

da59a79

3304: Extend deserialiseFromRawBytesHex to produce error description r=cblp a=cblp Co-authored-by: Yuriy Syrovetskiy <yuriy.syrovetskiy@iohk.io>

dcoutts reviewed Apr 28, 2022

View reviewed changes

MarcFontaine approved these changes Apr 29, 2022

View reviewed changes

cblp added 3 commits April 29, 2022 18:32

Extend deserialiseFromRawBytesHex to produce error description

2b1a604

Use updated deserialiseFromRawBytesHex

061970c

Simplify code using Parsec

beae391

cblp force-pushed the cblp/deserialiseFromRawBytesHex branch from 87645b3 to beae391 Compare April 29, 2022 16:43

iohk-bors bot merged commit 567ee70 into master May 2, 2022

iohk-bors bot deleted the cblp/deserialiseFromRawBytesHex branch May 2, 2022 16:16

cblp moved this from Review to Merged in cardano-node/cardano-api/cardano-cli May 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend deserialiseFromRawBytesHex to produce error description #3304

Extend deserialiseFromRawBytesHex to produce error description #3304

cblp commented Oct 20, 2021

Jimbo4350 left a comment

cblp commented Dec 23, 2021

Jimbo4350 left a comment

Jimbo4350 Dec 31, 2021

cblp Jan 11, 2022

Jimbo4350 Jan 17, 2022

cblp commented Mar 24, 2022

iohk-bors bot commented Mar 24, 2022

cblp commented Apr 22, 2022

iohk-bors bot commented Apr 22, 2022

cblp commented Apr 27, 2022

iohk-bors bot commented Apr 27, 2022

dcoutts Apr 28, 2022

cblp Apr 28, 2022

dcoutts Apr 28, 2022

cblp Apr 29, 2022

dcoutts Apr 28, 2022

MarcFontaine left a comment

cblp commented May 2, 2022

iohk-bors bot commented May 2, 2022

Extend deserialiseFromRawBytesHex to produce error description #3304

Extend deserialiseFromRawBytesHex to produce error description #3304

Conversation

cblp commented Oct 20, 2021

Jimbo4350 left a comment

Choose a reason for hiding this comment

cblp commented Dec 23, 2021

Jimbo4350 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cblp commented Mar 24, 2022

iohk-bors bot commented Mar 24, 2022

cblp commented Apr 22, 2022

iohk-bors bot commented Apr 22, 2022

cblp commented Apr 27, 2022

iohk-bors bot commented Apr 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcFontaine left a comment

Choose a reason for hiding this comment

cblp commented May 2, 2022

iohk-bors bot commented May 2, 2022