Split serialisation from IO #5049

newhoggy · 2023-04-04T07:40:30Z

We have functions that combined IO and serialisation. For example writeFileTextEnvelope.

This PR takes a first step in separating them so that we do IO and serialisation separately.

The separation allows the IO code and the serialisation to independently become more complicated without compounding each other.

For example on the serialisation side, we may support more serialisation formats and on the IO side we may support pipes and special "-" files.

This code simplifies #5058 and moves us towards #5016

carbolymer · 2023-04-04T09:04:14Z

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

+writeByteStringFile fp bs = runExceptT $
+  handleIOExceptT (FileIOError fp) $ BS.writeFile fp bs
+
+writeByteStringOutput :: MonadIO m => Maybe FilePath -> ByteString -> m (Either (FileError ()) ())


The first argument is a bit unclear, it takes a moment to understand that Nothing means stdout. Maybe encoding that information in the type would be better? For example:

data OutputFileDescriptor = OutputFile FilePath | OutputStream Handle

where Handle can be stdout or stderr (which doesn't really matter here).

Consequently, if you open a handle to a file path, I think the other functions, which then could operate on Handles, could be unified and simplified I think (instead of matching Maybe in every one of them).

There have been multiple attempts to do such a thing, but they've been piecemeal.

The purpose of this PR is not to introduce such an abstraction. We already have code that uses Maybe FilePath and keeping this code to that convention minimises difference.

We may introduce an appropriate abstraction in a separate PR and do it across the entire codebase to handle consistently.

We may introduce an appropriate abstraction in a separate PR and do it across the entire codebase to handle consistently.

@carbolymer this would be a good issue to create and tackle next.

I've created: #5062

carbolymer · 2023-04-04T09:13:54Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Key.hs

@@ -244,8 +244,9 @@ runConvertToNonExtendedKey evkf (VerificationKeyFile vkf) =
  writeToDisk


I think this function could be expanded to accept also error type constructor and could be used everywhere when the:

firstExceptT e . newExceptT $ writeLazyByteStringFile vkf' $ textEnvelopeToJSON desc vk

is used now.

I prefer not to mingle functional code with error handling if possible.

In some of the changes I've kept the firstExceptT e . newExceptT rather than grouping them also to minimise changes.

We can group them to simplify the code.

Jimbo4350

Couple minor changes

Jimbo4350 · 2023-04-05T19:58:07Z

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

+writeByteStringFile fp bs = runExceptT $
+  handleIOExceptT (FileIOError fp) $ BS.writeFile fp bs
+
+writeByteStringOutput :: MonadIO m => Maybe FilePath -> ByteString -> m (Either (FileError ()) ())


We may introduce an appropriate abstraction in a separate PR and do it across the entire codebase to handle consistently.

@carbolymer this would be a good issue to create and tackle next.

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

cardano-api/src/Cardano/Api/IO/Compat.hs

Comments addressed

* writeByteStringFile * writeByteStringOutput * writeLazyByteStringFile * writeLazyByteStringOutput * writeTextFile * writeTextOutput

…m IO

…ardano.Api.SerialiseTextEnvelope has less IO code and can be freed from CPP

…nerPermissions for consistency

…orWritingWithOwnerPermission

* writeByteStringFileWithOwnerPermissions * writeTextFileWithOwnerPermissions

…ringFileWithOwnerPermissions and textEnvelopeToJSON instead

Jimbo4350

LGTM!

newhoggy force-pushed the master branch from 7f1dd84 to 27f8437 Compare April 4, 2023 08:09

newhoggy force-pushed the newhoggy/split-serialisation-from-io branch from 93bfd86 to ed76cf9 Compare April 4, 2023 08:12

newhoggy marked this pull request as ready for review April 4, 2023 08:24

newhoggy requested review from dcoutts, Jimbo4350, erikd, JaredCorduan and CarlosLopezDeLara as code owners April 4, 2023 08:24

newhoggy changed the title ~~Split serialisation from io~~ Split serialisation from IO Apr 4, 2023

carbolymer reviewed Apr 4, 2023

View reviewed changes

carbolymer approved these changes Apr 4, 2023

View reviewed changes

newhoggy mentioned this pull request Apr 5, 2023

Option to write keys in bech32 #5058

Merged

Jimbo4350 previously requested changes Apr 5, 2023

View reviewed changes

newhoggy force-pushed the newhoggy/split-serialisation-from-io branch from b54b187 to cb74808 Compare April 6, 2023 03:02

newhoggy requested a review from Jimbo4350 April 6, 2023 03:08

newhoggy added 5 commits April 6, 2023 13:08

Move OutputFile to cardano-api

3b967ee

New functions for writing to file and stdout:

c9fe62b

* writeByteStringFile * writeByteStringOutput * writeLazyByteStringFile * writeLazyByteStringOutput * writeTextFile * writeTextOutput

Split callsite of writeFileTextEnvelope to separate serialisation fro…

2e5adc2

…m IO

Move writeFileWithOwnerPermissions to Cardano.Api.IO.Compat so that C…

15a843d

…ardano.Api.SerialiseTextEnvelope has less IO code and can be freed from CPP

Rename writeFileWithOwnerPermissions to writeLazyByteStringFileWithOw…

7b91bf5

…nerPermissions for consistency

newhoggy force-pushed the newhoggy/split-serialisation-from-io branch 5 times, most recently from 46e17ec to 9caffc2 Compare April 6, 2023 05:28

newhoggy added 3 commits April 6, 2023 15:32

Generalise writeLazyByteStringFileWithOwnerPermissions to handleFileF…

971156d

…orWritingWithOwnerPermission

New functions for completeness:

9076213

* writeByteStringFileWithOwnerPermissions * writeTextFileWithOwnerPermissions

Remove writeFileTextEnvelopeWithOwnerPermissions. Use writeLazyByteSt…

2d11855

…ringFileWithOwnerPermissions and textEnvelopeToJSON instead

newhoggy force-pushed the newhoggy/split-serialisation-from-io branch from 9caffc2 to 2d11855 Compare April 6, 2023 05:32

Jimbo4350 approved these changes Apr 6, 2023

View reviewed changes

newhoggy added this pull request to the merge queue Apr 6, 2023

Merged via the queue into master with commit d456afb Apr 6, 2023

iohk-bors bot deleted the newhoggy/split-serialisation-from-io branch April 6, 2023 16:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split serialisation from IO #5049

Split serialisation from IO #5049

newhoggy commented Apr 4, 2023 •

edited

Loading

carbolymer Apr 4, 2023 •

edited

Loading

newhoggy Apr 4, 2023 •

edited

Loading

Jimbo4350 Apr 5, 2023

carbolymer Apr 5, 2023

carbolymer Apr 4, 2023 •

edited

Loading

newhoggy Apr 6, 2023

Jimbo4350 left a comment •

edited

Loading

Jimbo4350 Apr 5, 2023

Jimbo4350 left a comment

		@@ -244,8 +244,9 @@ runConvertToNonExtendedKey evkf (VerificationKeyFile vkf) =
		writeToDisk

Split serialisation from IO #5049

Split serialisation from IO #5049

Conversation

newhoggy commented Apr 4, 2023 • edited Loading

carbolymer Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

newhoggy Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 Apr 5, 2023

Choose a reason for hiding this comment

carbolymer Apr 5, 2023

Choose a reason for hiding this comment

carbolymer Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

newhoggy Apr 6, 2023

Choose a reason for hiding this comment

Jimbo4350 left a comment • edited Loading

Choose a reason for hiding this comment

Jimbo4350 Apr 5, 2023

Choose a reason for hiding this comment

Jimbo4350 left a comment

Choose a reason for hiding this comment

newhoggy commented Apr 4, 2023 •

edited

Loading

carbolymer Apr 4, 2023 •

edited

Loading

newhoggy Apr 4, 2023 •

edited

Loading

carbolymer Apr 4, 2023 •

edited

Loading

Jimbo4350 left a comment •

edited

Loading