Read networkId from environment #4666

MarcFontaine · 2022-11-25T15:28:21Z

Read network ID from environment.
Two formats are supported

CARDANO_NODE_NETWORK_ID=mainnet
CARDANO_NODE_NETWORK_ID=testnet-xx
where xx parses as Word32

MarcFontaine · 2022-11-25T15:28:56Z

CarlosLopezDeLara · 2022-12-13T17:55:16Z

dcouts jimbo can you guys help me please im learning fast but i seriously need some help to retreave my byron wallet and rewards i tried to code the other day for the first time ever

For support please reach out to our support team https://iohk.zendesk.com/hc/en-us/requests/new

Jimbo4350 · 2022-12-14T13:50:00Z

@FonziFonzarelli Open an issue or go the the link @CarlosLopezDeLara pasted above. Do not hijack open PRs, you will not receive help.

cardano-api/src/Cardano/Api.hs

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

Jimbo4350

Nice, a few comments.

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

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

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

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

Jimbo4350 · 2023-01-03T18:58:30Z

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

+displayErrorText :: Error e => e -> Text
+displayErrorText = Text.pack . displayError
+
+rescue :: Functor m =>  m (Either x a) -> (x -> y) -> ExceptT y m a


rescue is a weird name for this. What about firstNewExceptT? That's a bit more indicative and you should consider swapping around the parameters if it doesn't make its usage awkward i.e firstNewExceptT :: Functor m => (x -> y) -> m (Either x a) -> ExceptT y m a

It is called rescue in Ruby.
I like rescue much better than firstNewExceptT .
Also it is more readable to add " `rescue` ErrorTag" trailing at the end of the line.
Then the good- case comes first and error handling is less distracting when reading the code.
(The rescue pattern exists in several other languages..)

rescue is still vague IMO and because it exists in other languages doesn't necessarily mean it's a good name. What about liftMapError? Let's get @newhoggy 's input on this

For me rescue is much easier to read and type than liftMapError. 7 vs 12 char and no need for Caps.
Some Haskell guys would even introduce a &%> style operator for that.
For and old coder, who needs thick glasses and uses ever bigger font sizes readability is important.
Some IDEs even show the type when hovering the function.

This comment is a bit tangential but relevant.

I had something thoughts about error handling in general.

A function like rescue seems generic - but then if it is generic it should go in some generic error handling module, but when we do that we might eventually encounter a problem:

A function like rescue seems fine by itself, but in practise, we might need a family of functions and would then have trouble naming them. For example:

rescue :: Functor m => m (Either x a) -> (x -> y) -> ExceptT y m a rescue :: Functor m => Either x a -> (x -> y) -> ExceptT y m a rescue :: Functor m => ExceptT x m a -> (x -> y) -> ExceptT y m a

I was unhappy with my earlier PR that introduced onNothingThrow for this reason. It was defined like this:

onNothingThrow :: Monad m => e -> Maybe a -> ExceptT e m a

But then what about all these other possibilities?

onNothingThrow :: Monad m => e -> Maybe a -> ExceptT e m a onNothingThrow :: Monad m => e -> m (Maybe a) -> ExceptT e m a onNothingThrow :: Monad m => e -> ExceptT e m (Maybe a) -> ExceptT e m a

I decided I only need the last one because everything else can be lifted to it.

See this PR: #4807

Interested in your thoughts.

(Note, I'm intending to restructure oops to use more readable English names and avoid the explosion of permutations in a similar way)

Nevermind I closed the PR and favouring hoistMaybe instead.

I suggest we don't define rescue and should instead inline it:

getNetworkId :: NetworkIdArg -> (EnvNetworkIdError -> e) -> ExceptT e IO NetworkId getNetworkId arg errorTag = firstExceptT errorTag $ ExceptT $ getNetworkId' arg

I agree with defining it inline.

There is not one use site of the rescue function but there are about ca 100 possible use sites.
This is the 101 of programming: if there are multiple repetitions of the same pattern one defines a function.
Otherwise the code becomes extremely ugly and hard to maintain.
(As is already the case.)
If there is a need for a family of functions which all do something similar like lifting and error handling
then that is indeed a case for a type class.
It makes no sense at all to just refuse to add any kind of abstraction, just because people cannot agree on a common name for a function.
Once an abstraction is in place, it is very easy to rename the function.
It is better to start with a simple abstraction and think about a type classs later.

I had something thoughts about error handling in general.
A function like rescue seems generic - but then if it is generic it should go in some generic error handling
module, but when we do that we might eventually encounter a problem:
A function like rescue seems fine by itself, but in practise, we might need a family of functions and would then
have trouble naming them. For example:

This kind of argument is well know and it goes by the phrase:
"Perfect is the enemy of good"

As stated earlier, there are lots and lots of places in the code that could be simplified by using rescue.
If there is need for a more elegant and more generic solution in the future
then replacing the repetitive pattern with a function like rescue now is still a very good step in that direction.

Jimbo4350 · 2023-01-03T19:05:34Z

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

+  NetworkIdFromCLI x -> return $ Right x
+  NetworkIdFromEnv -> readEnvNetworkId
+
+getNetworkId :: NetworkIdArg -> (EnvNetworkIdError -> e) -> ExceptT e IO NetworkId


We should export getNetworkId' as getNetworkId. Leave it as a vanilla function with no ExceptT usage. Then we should rename getNetworkId to firstGetNetworkId or something like that to make it more obvious that we are transforming the error.

Primed names can be problematic in general, but here is no danger of ever using getNetworkId' instead of getNetworkId because they have different types.
getNetworkId is the primary function with 47 call sites.
getNetworkId' an auxiliary function with 7 call sites.
The simple name should be used for the primary function with more call sites
and the more complex name for the auxiliary function with fewer call sites.

getNetworkId is specialized for our use case in the cli. This should be renamed to indicate we are mapping over the error hence the name firstGetNetworkId. These functions are likely to end up in cardano-api, which is why I am suggesting we rename getNetworkId' :: NetworkIdArg -> IO (Either EnvNetworkIdError NetworkId) to getNetworkId. We should not impose our specific cli design decisions (ExceptT) on users.

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

Jimbo4350 · 2023-01-03T19:28:55Z

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

+--
+data SignForNetworkId
+  = SignForNetworkId !NetworkId
+  | TakeNetworkIdFromKey


Instead of creating a new sum type and new parsers why not just use Maybe NetworkId like before? SignForNetworkId doesn't really bring much value over Maybe NetworkId. Also the constructor TakeNetworkIdFromKey is misleading.

What you suggest is the situation as it is.
And this situation is very unsatisfactory because at the moment Maybe NetworkId is used with two different meanings and two different interpretations in the (existing) code.
Some of the commands resolve Maybe NetworkId using the network ID from the environment or the flag.
But there is also the one that takes the network ID from the key in the Nothing case.
The best way to handle that is to create a new data type in Haskell.
That is very cheap and helps to cleanup stuff a lot.
It is a common pattern to use
data MyFeature = HaveFeature | DontHaveFeature
instead of
type MyFeature = Bool

Sure but I doubt it's worth further increasing the complexity + size of the codebase to differentiate between these two cases. We already have numerous data definitions in the cli and I don't want to introduce anymore unless the benefit is significant. I don't see that being the case here.

It is much more readable, adds very little overhead and helps to avoid bugs.
When adding support for networkId from env it was necessary to differentiate between these two cases.
if done right (!) it can even help to reduce code complexity and code size.
Also can help to get better help messages in the CLI.

Here is an example.
Maybe NetworkId is defaulted to Mainnet right in the middle of some code.
With a proper datatype it would be easier to spot and avoid unwanted defaults.

The way to fix that is to change the function type signature from:

runTxCalculateMinFee :: TxBodyFile -> Maybe NetworkId

to

runTxCalculateMinFee :: TxBodyFile -> NetworkId

Which eliminates the need to add another data definition here because we don't want defaults.

cardano-api/src/Cardano/Api.hs

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

Jimbo4350 · 2023-01-17T19:32:34Z

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

+--
+data SignForNetworkId
+  = SignForNetworkId !NetworkId
+  | TakeNetworkIdFromKey


Sure but I doubt it's worth further increasing the complexity + size of the codebase to differentiate between these two cases. We already have numerous data definitions in the cli and I don't want to introduce anymore unless the benefit is significant. I don't see that being the case here.

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

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

Jimbo4350 · 2023-01-17T19:39:30Z

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

+displayErrorText :: Error e => e -> Text
+displayErrorText = Text.pack . displayError
+
+rescue :: Functor m =>  m (Either x a) -> (x -> y) -> ExceptT y m a


rescue is still vague IMO and because it exists in other languages doesn't necessarily mean it's a good name. What about liftMapError? Let's get @newhoggy 's input on this

Jimbo4350 · 2023-01-17T19:44:50Z

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

+  NetworkIdFromCLI x -> return $ Right x
+  NetworkIdFromEnv -> readEnvNetworkId
+
+getNetworkId :: NetworkIdArg -> (EnvNetworkIdError -> e) -> ExceptT e IO NetworkId


getNetworkId is specialized for our use case in the cli. This should be renamed to indicate we are mapping over the error hence the name firstGetNetworkId. These functions are likely to end up in cardano-api, which is why I am suggesting we rename getNetworkId' :: NetworkIdArg -> IO (Either EnvNetworkIdError NetworkId) to getNetworkId. We should not impose our specific cli design decisions (ExceptT) on users.

Two formats are supported * CARDANO_NODE_NETWORK_ID=mainnet * CARDANO_NODE_NETWORK_ID=testnet-xx where xx parses as Word32

newhoggy · 2023-04-26T06:50:54Z

Implemented in #5119 so closing this PR

MarcFontaine self-assigned this Nov 25, 2022

MarcFontaine added the WIP Work In Progress (cannot be merged yet) label Nov 25, 2022

MarcFontaine force-pushed the mafo/4418-networkId-from-env branch 3 times, most recently from e9cf022 to 03d1119 Compare November 30, 2022 15:35

MarcFontaine force-pushed the mafo/4418-networkId-from-env branch from 03d1119 to 4b9a6c5 Compare December 13, 2022 15:45

MarcFontaine marked this pull request as ready for review December 13, 2022 15:46

MarcFontaine requested review from dcoutts, Jimbo4350, erikd, newhoggy, JaredCorduan, LudvikGalois and CarlosLopezDeLara as code owners December 13, 2022 15:46

MarcFontaine force-pushed the mafo/4418-networkId-from-env branch 4 times, most recently from 7416ee9 to 771c004 Compare December 14, 2022 13:46

IntersectMBO deleted a comment Dec 14, 2022

MarcFontaine removed the WIP Work In Progress (cannot be merged yet) label Dec 14, 2022

Jimbo4350 requested changes Dec 14, 2022

View reviewed changes

cardano-api/src/Cardano/Api.hs Show resolved Hide resolved

cardano-api/src/Cardano/Api/Environment.hs Outdated Show resolved Hide resolved

MarcFontaine force-pushed the mafo/4418-networkId-from-env branch 2 times, most recently from 99a54fa to 14c9fe0 Compare December 22, 2022 13:52

MarcFontaine requested a review from Jimbo4350 December 22, 2022 14:08

MarcFontaine force-pushed the mafo/4418-networkId-from-env branch from 14c9fe0 to 837f998 Compare January 3, 2023 15:33

Jimbo4350 requested changes Jan 3, 2023

View reviewed changes

IntersectMBO deleted a comment Jan 3, 2023

MarcFontaine force-pushed the mafo/4418-networkId-from-env branch from 837f998 to a8c491d Compare January 6, 2023 14:24

MarcFontaine requested a review from Jimbo4350 January 6, 2023 17:03

MarcFontaine force-pushed the mafo/4418-networkId-from-env branch from a8c491d to 6a5fe53 Compare January 9, 2023 13:06

Jimbo4350 requested changes Jan 17, 2023

View reviewed changes

newhoggy mentioned this pull request Jan 18, 2023

Refactor onNothingThrow #4807

Closed

Read network ID from environment

41c9003

Two formats are supported * CARDANO_NODE_NETWORK_ID=mainnet * CARDANO_NODE_NETWORK_ID=testnet-xx where xx parses as Word32

MarcFontaine force-pushed the mafo/4418-networkId-from-env branch from 6a5fe53 to 41c9003 Compare January 31, 2023 13:56

newhoggy closed this Apr 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read networkId from environment #4666

Read networkId from environment #4666

MarcFontaine commented Nov 25, 2022 •

edited

Loading

MarcFontaine commented Nov 25, 2022

CarlosLopezDeLara commented Dec 13, 2022

Jimbo4350 commented Dec 14, 2022

Jimbo4350 left a comment •

edited

Loading

Jimbo4350 Jan 3, 2023

MarcFontaine Jan 5, 2023

Jimbo4350 Jan 17, 2023

MarcFontaine Jan 17, 2023 •

edited

Loading

newhoggy Jan 18, 2023

newhoggy Jan 18, 2023 •

edited

Loading

newhoggy Jan 18, 2023

Jimbo4350 Jan 23, 2023

MarcFontaine Jan 24, 2023

MarcFontaine Jan 31, 2023

Jimbo4350 Jan 3, 2023

MarcFontaine Jan 5, 2023

Jimbo4350 Jan 17, 2023

Jimbo4350 Jan 3, 2023 •

edited

Loading

MarcFontaine Jan 5, 2023

Jimbo4350 Jan 17, 2023

MarcFontaine Jan 17, 2023

MarcFontaine Jan 31, 2023

Jimbo4350 Feb 7, 2023 •

edited

Loading

Jimbo4350 Jan 17, 2023

Jimbo4350 Jan 17, 2023

Jimbo4350 Jan 17, 2023

newhoggy commented Apr 26, 2023

Read networkId from environment #4666

Read networkId from environment #4666

Conversation

MarcFontaine commented Nov 25, 2022 • edited Loading

MarcFontaine commented Nov 25, 2022

CarlosLopezDeLara commented Dec 13, 2022

Jimbo4350 commented Dec 14, 2022

Jimbo4350 left a comment • edited Loading

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 Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newhoggy Jan 18, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 Jan 3, 2023 • edited Loading

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

Jimbo4350 Feb 7, 2023 • edited Loading

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

newhoggy commented Apr 26, 2023

MarcFontaine commented Nov 25, 2022 •

edited

Loading

Jimbo4350 left a comment •

edited

Loading

MarcFontaine Jan 17, 2023 •

edited

Loading

newhoggy Jan 18, 2023 •

edited

Loading

Jimbo4350 Jan 3, 2023 •

edited

Loading

Jimbo4350 Feb 7, 2023 •

edited

Loading