[CO-410] Add `RequiresNetworkMagic` and modify `ProtocolMagic` #3715

intricate · 2018-10-05T03:05:37Z

Description

This PR modifies the ProtocolMagic data structure and introduces new data types: RequiresNetworkMagic and ProtocolMagicId (a new type wrapper around an Int32).

ProtocolMagic has been modified such that it consists of two fields: getProtocolMagicId :: ProtocolMagicId and getRequiresNetworkMagic :: RequiresNetworkMagic.

The tests which were introduced in #3712 have assisted us in ensuring that we've maintained backwards compatibility in our implementation of ProtocolMagic.

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CO-410

Type of change

🐞 Bug fix (non-breaking change which fixes an issue)
🛠 New feature (non-breaking change which adds functionality)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)
🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
🔨 New or improved tests for existing code
⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

I have read the style guide document, and my code follows the code style of this project.
If my code deals with exceptions, it follows the guidelines.
I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.

Testing checklist

I have added tests to cover my changes.
All new and existing tests passed.

QA Steps

Screenshots (if available)

mhuesch

FromJSON instances seem off to me... let me know if I'm misunderstanding though.

chain/bench/block-bench.hs

chain/test/Test/Pos/Chain/Genesis/Dummy.hs

mhuesch · 2018-10-10T15:35:03Z

crypto/Pos/Crypto/Configuration.hs

+                                 <*> pure RequiresMagic
+            Just (A.String "NMMustBeNothing")
+                -> ProtocolMagic <$> (ProtocolMagicId <$> o .: "pm")
+                                 <*> pure RequiresNoMagic


Is there a reason that this legacy-migration logic is in the FromJSON ProtocolMagic instance, rather than directly in the FromJSON RequiresNetworkMagic instance?

Do we have an issue here and here with supporting the legacy names? It seems like those use the FromJSON RequiresNetworkMagic instance, which will not be equipped to handle the legacy names.

You're absolutely right, there will be issues in the FromJSON instance of Configuration . I'll make the necessary changes.

crypto/test/Test/Pos/Crypto/Json.hs

lib/test/Test/Pos/Launcher/ConfigurationSpec.hs

mhuesch · 2018-10-10T15:43:56Z

lib/test/Test/Pos/Launcher/ConfigurationSpec.hs

            startTime <- Timestamp . round . (* 1000000) <$> liftIO getPOSIXTime
            let cfo = defaultConfigurationOptions
-                        { cfoFilePath = "./configuration.yaml"
+                        { cfoFilePath = configFilePath


The "./" prefix might be important in some contexts, I would keep it in there.

@Jimbo4350 asked about this - I'm not sure that presence/absence of a "./ is an issue here, but given that the code has been working with the prefix, I'd hesitate to casually drop it. I'm not sure that all architectures treat said prefixes similarly.

My suggestion is:

{ cfoFilePath = "./" <> configFilePath

…rt legacy encoding

`systemTag` in `Configuration_Legacy` golden file

mhuesch

LGTM - good work!

The path issue is likely not a big deal. We can change it or leave it.

lib/test/Test/Pos/Launcher/ConfigurationSpec.hs

mhuesch · 2018-10-15T17:03:50Z

lib/test/Test/Pos/Launcher/Json.hs

+        , ccExplorerExtendedApi = False
+        }
+    , ccWallet = WalletConfiguration { ccThrottle = Nothing }
+    , ccReqNetMagic = RequiresNoMagic


Had to run this through in my head, but it seems that we're pulling the "requiresNetworkMagic":"RequiresNoMagic" field from JSON into ccReqNetMagic, and the ccGenesis.gsProtocolConstants.gpcProtocolMagic.getRequiresNetworkMagic value is defaulting because the JSON has "protocolMagic":55550001. This looks right 👍

Thanks for adding this, @intricate.

* [CO-410] Port release/1.3.1 config to develop * [CO-410] Modify ProtocolMagic * [CO-410] Continue modifying ProtocolMagic * [CO-410] Change naming * [CO-410] Add golden files for legacy `RequiresNetworkMagic` * [CO-410] Modify `RequiresNetworkMagic`'s `FromJSON` instance to support legacy encoding * [CO-410] Add golden decoding tests for legacy `ProtocolMagic` encoding * [CO-410] Add golden decode only test for `Configuration` * [CO-410] Run `pkgs/generate.sh`, remove reference files, hardcode `systemTag` in `Configuration_Legacy` golden file

…-output-hk/cardano-sl#3715) * [CO-410] Port release/1.3.1 config to develop * [CO-410] Modify ProtocolMagic * [CO-410] Continue modifying ProtocolMagic * [CO-410] Change naming * [CO-410] Add golden files for legacy `RequiresNetworkMagic` * [CO-410] Modify `RequiresNetworkMagic`'s `FromJSON` instance to support legacy encoding * [CO-410] Add golden decoding tests for legacy `ProtocolMagic` encoding * [CO-410] Add golden decode only test for `Configuration` * [CO-410] Run `pkgs/generate.sh`, remove reference files, hardcode `systemTag` in `Configuration_Legacy` golden file

intricate added wip DO NOT MERGE labels Oct 5, 2018

intricate assigned Jimbo4350 and intricate Oct 5, 2018

intricate force-pushed the i+m/develop/CO-410/modify-protocolmagic branch 6 times, most recently from 0b8c020 to 7644bd3 Compare October 8, 2018 12:34

intricate changed the title ~~[DO NOT MERGE] [WIP] [CO-410] Add RequiresNetworkMagic and modify ProtocolMagic~~ [CO-410] Add RequiresNetworkMagic and modify ProtocolMagic Oct 8, 2018

intricate removed DO NOT MERGE wip labels Oct 8, 2018

intricate requested review from Jimbo4350, ruhatch, mhuesch and erikd October 8, 2018 13:02

Jimbo4350 force-pushed the i+m/develop/CO-410/modify-protocolmagic branch 3 times, most recently from 4b9ee6a to e2f59a8 Compare October 10, 2018 12:56

intricate force-pushed the i+m/develop/CO-410/modify-protocolmagic branch from e2f59a8 to ad5438c Compare October 10, 2018 13:38

intricate mentioned this pull request Oct 10, 2018

[CO-410] Introduce NetworkMagic #3733

Merged

5 tasks

mhuesch suggested changes Oct 10, 2018

View reviewed changes

Jimbo4350 force-pushed the i+m/develop/CO-410/modify-protocolmagic branch 7 times, most recently from 2296464 to 66e820c Compare October 12, 2018 13:24

Jimbo4350 force-pushed the i+m/develop/CO-410/modify-protocolmagic branch from 66e820c to dcb500b Compare October 12, 2018 13:36

intricate force-pushed the i+m/develop/CO-410/modify-protocolmagic branch from 2e1b589 to 61a7eeb Compare October 12, 2018 14:39

Jimbo4350 force-pushed the i+m/develop/CO-410/modify-protocolmagic branch 9 times, most recently from 1eafd3f to 5d1317a Compare October 12, 2018 17:10

mhuesch and others added 8 commits October 15, 2018 08:23

[CO-410] Port release/1.3.1 config to develop

621230a

[CO-410] Modify ProtocolMagic

2183a57

[CO-410] Continue modifying ProtocolMagic

3968b3b

[CO-410] Change naming

884bff3

[CO-410] Add golden files for legacy RequiresNetworkMagic

a893181

[CO-410] Modify RequiresNetworkMagic's FromJSON instance to suppo…

06a95a3

…rt legacy encoding

[CO-410] Add golden decoding tests for legacy ProtocolMagic encoding

f545fc8

[CO-410] Add golden decode only test for Configuration

56e4016

Jimbo4350 force-pushed the i+m/develop/CO-410/modify-protocolmagic branch 2 times, most recently from c0972b1 to 7fb6ab3 Compare October 15, 2018 14:00

[CO-410] Run pkgs/generate.sh, remove reference files, hardcode

a9309aa

`systemTag` in `Configuration_Legacy` golden file

Jimbo4350 force-pushed the i+m/develop/CO-410/modify-protocolmagic branch from 7fb6ab3 to a9309aa Compare October 15, 2018 14:22

mhuesch approved these changes Oct 15, 2018

View reviewed changes

intricate merged commit ccca8b3 into develop Oct 15, 2018

intricate deleted the i+m/develop/CO-410/modify-protocolmagic branch October 16, 2018 14:37

intricate mentioned this pull request Mar 22, 2019

Implement address discrimination via NetworkMagic input-output-hk/cardano-ledger-byron#385

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CO-410] Add `RequiresNetworkMagic` and modify `ProtocolMagic` #3715

[CO-410] Add `RequiresNetworkMagic` and modify `ProtocolMagic` #3715

intricate commented Oct 5, 2018

mhuesch left a comment

mhuesch Oct 10, 2018

Jimbo4350 Oct 10, 2018 •

edited

Loading

mhuesch Oct 10, 2018

mhuesch Oct 15, 2018

mhuesch Oct 15, 2018

mhuesch left a comment

mhuesch Oct 15, 2018

mhuesch Oct 15, 2018

[CO-410] Add RequiresNetworkMagic and modify ProtocolMagic #3715

[CO-410] Add RequiresNetworkMagic and modify ProtocolMagic #3715

Conversation

intricate commented Oct 5, 2018

Description

Linked issue

Type of change

Developer checklist

Testing checklist

QA Steps

Screenshots (if available)

mhuesch left a comment

Choose a reason for hiding this comment

mhuesch Oct 10, 2018

Choose a reason for hiding this comment

Jimbo4350 Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

mhuesch Oct 10, 2018

Choose a reason for hiding this comment

mhuesch Oct 15, 2018

Choose a reason for hiding this comment

mhuesch Oct 15, 2018

Choose a reason for hiding this comment

mhuesch left a comment

Choose a reason for hiding this comment

mhuesch Oct 15, 2018

Choose a reason for hiding this comment

mhuesch Oct 15, 2018

Choose a reason for hiding this comment

[CO-410] Add `RequiresNetworkMagic` and modify `ProtocolMagic` #3715

[CO-410] Add `RequiresNetworkMagic` and modify `ProtocolMagic` #3715

Jimbo4350 Oct 10, 2018 •

edited

Loading