[CO-410] Implement `NetworkMagic` logic #3775

intricate · 2018-10-23T04:51:54Z

Description

We pipe the full NetworkMagic to the "frontier" outlined above. This means bridging the gap from where we have ProtocolMagic to where we need NetworkMagic (and had RequiresNoMagic hardcoded).

Since PR #3756 splits the test-suites, we see most of the changes in this PR are to source-code files.

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)
^ this PR adds the capability for differently formatted Addresses, which include a NetworkMagic value. Thus it is a breaking change for testnets (NetworkTestnet), but backwards compatible for mainnet/staging (NetworkMainOrStage).

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.
[~] CHANGELOG entry has been added and is linked to the correct PR on GitHub.
^ there is no 1.3.1 section in the CHANGELOG, and this is dev work on a release branch, so I'm skipping this.

Testing checklist

[~] I have added tests to cover my changes.
All new and existing tests passed.
^ tests we added/modified in [CO-410] Split test-suites for NetworkMainOrStage/NetworkTestnet #3756 - we are relying on those.

Jimbo4350

Only looked at final commit (1204e85) as per @intricate's instruction. One minor comment.

Jimbo4350 · 2018-10-23T22:05:15Z

tools/src/Pos/Tools/Dbgen/Lib.hs

        Just accId ->
-            if checkIfAddTo fakeUtxoSpec fakeTxs then
-                addAddressesTo spec accId
+            if (checkIfAddTo fakeUtxoSpec fakeTxs) then


Redundant brackets here.

auxx/src/Command/Tx.hs

erikd

LGTM, but we should probably get one more set of eyes on this.

mhuesch

LGTM

tools/src/addr-convert/Main.hs

mhuesch · 2018-10-24T11:22:25Z

wallet-new/test/unit/Wallet/Inductive/Cardano.hs

-        nm <- asks (makeNetworkMagic . ccMagic . tcCardano)
+        let newRootId = HD.eskToHdRootId nm esk
+        -- TODO @intricate: Get from TransCtxt or PassiveWallet?
+        -- nm <- asks (makeNetworkMagic . ccMagic . tcCardano)


Stale comment?

Oops. Thanks.

wallet/test/Test/Pos/Wallet/Web/Methods/PaymentSpec.hs

wallet/test/Test/Pos/Wallet/Web/Tracking/SyncSpec.hs

parsonsmatt · 2018-10-24T14:07:10Z

db/src/Pos/DB/Block/Slog/Logic.hs

    -- in BlockDB), we will have garbage blunds in BlockDB, but it's not a
    -- problem.
-    bListenerBatch <- if callBListener then onApplyBlocks fixedNM blunds
+    let nm = makeNetworkMagic pm


If the only thing we do with the protocolMagic value is create a networkMagic from it, why not accept it directly?

My mistake.

parsonsmatt · 2018-10-24T14:08:12Z

lib/src/Pos/Util/UserSecret.hs

        bprint ("{ root = "%addressF%", set name = "%build%
                ", wallets = "%pairsF%", accounts = "%pairsF%" }")
-        (makeRootPubKeyAddress fixedNM $ encToPublic _wusRootKey)
+        (makeRootPubKeyAddress NetworkMainOrStage $ encToPublic _wusRootKey)


Correct me if I am wrong: won't this return a wrong address result for testing networks?

Yes, you're correct. This slipped my mind as I was supposed to come back and address this. I may need to add a NetworkMagic field to WalletUserSecret. However, I wonder if that's a good idea given that this NetworkMagic field within WalletUserSecret will never be used in any place other than its Buildable instance. Will look into either changing the output from build or adding this field.

Decided to change the Buildable instance such that the public key is printed instead of the address.

tools/src/addr-convert/Main.hs

Co-authored-by: Michael Hueschen <[email protected]> Co-authored-by: Jordan Millar <[email protected]>

…ement-networkmagic [CO-410] Implement `NetworkMagic` logic

…hk/i+j/develop/CO-410/implement-networkmagic [CO-410] Implement `NetworkMagic` logic

intricate added wip DO NOT MERGE labels Oct 23, 2018

intricate assigned intricate and Jimbo4350 Oct 23, 2018

intricate mentioned this pull request Oct 23, 2018

[CO-410] Split test-suites for NetworkMainOrStage/NetworkTestnet #3756

Merged

7 tasks

Jimbo4350 reviewed Oct 23, 2018

View reviewed changes

intricate force-pushed the i+j/develop/CO-410/implement-networkmagic branch from 1204e85 to 39dfb58 Compare October 24, 2018 00:10

intricate removed the wip label Oct 24, 2018

intricate changed the title ~~[DO NOT MERGE] [WIP] [CO-410] Implement NetworkMagic logic~~ [DO NOT MERGE] [CO-410] Implement NetworkMagic logic Oct 24, 2018

intricate requested review from erikd, mhuesch and ruhatch October 24, 2018 00:16

intricate force-pushed the i+j/develop/CO-410/implement-networkmagic branch 2 times, most recently from 038f8d5 to fdf3427 Compare October 24, 2018 01:59

intricate changed the title ~~[DO NOT MERGE] [CO-410] Implement NetworkMagic logic~~ [CO-410] Implement NetworkMagic logic Oct 24, 2018

erikd reviewed Oct 24, 2018

View reviewed changes

auxx/src/Command/Tx.hs Show resolved Hide resolved

intricate removed the DO NOT MERGE label Oct 24, 2018

intricate force-pushed the i+j/develop/CO-410/implement-networkmagic branch from fdf3427 to 24aec7f Compare October 24, 2018 02:15

erikd approved these changes Oct 24, 2018

View reviewed changes

intricate requested review from disassembler and parsonsmatt October 24, 2018 03:19

mhuesch approved these changes Oct 24, 2018

View reviewed changes

intricate force-pushed the i+j/develop/CO-410/implement-networkmagic branch from 0e90771 to 7a38603 Compare October 24, 2018 14:11

parsonsmatt approved these changes Oct 24, 2018

View reviewed changes

intricate force-pushed the i+j/develop/CO-410/implement-networkmagic branch 3 times, most recently from 9bacd05 to e5722ad Compare October 24, 2018 16:23

[CO-410] Implement NetworkMagic

80e3d99

Co-authored-by: Michael Hueschen <[email protected]> Co-authored-by: Jordan Millar <[email protected]>

intricate force-pushed the i+j/develop/CO-410/implement-networkmagic branch from e5722ad to 80e3d99 Compare October 25, 2018 14:18

intricate merged commit 2160095 into develop Oct 25, 2018

intricate deleted the i+j/develop/CO-410/implement-networkmagic branch October 25, 2018 14:47

intricate mentioned this pull request Nov 2, 2018

[release/2.0.0] [RCD-40] Address discrimination not enforced when sending transactions #3819

Merged

11 tasks

KtorZ pushed a commit that referenced this pull request Nov 9, 2018

Merge pull request #3775 from input-output-hk/i+j/develop/CO-410/impl…

1b7ad20

…ement-networkmagic [CO-410] Implement `NetworkMagic` logic

KtorZ pushed a commit to input-output-hk/cardano-wallet-legacy that referenced this pull request Nov 9, 2018

Merge pull request input-output-hk/cardano-sl#3775 from input-output-…

c80d71f

…hk/i+j/develop/CO-410/implement-networkmagic [CO-410] Implement `NetworkMagic` logic

intricate mentioned this pull request Mar 22, 2019

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

Closed

[CO-410] Implement NetworkMagic logic #3775

[CO-410] Implement NetworkMagic logic #3775

Uh oh!

Conversation

intricate commented Oct 23, 2018

Description

Linked issue

Type of change

Developer checklist

Testing checklist

Uh oh!

Jimbo4350 left a comment

Choose a reason for hiding this comment

Uh oh!

Jimbo4350 Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

intricate Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erikd left a comment

Choose a reason for hiding this comment

Uh oh!

mhuesch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhuesch Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

intricate Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

parsonsmatt Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

intricate Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parsonsmatt Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

intricate Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

intricate Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[CO-410] Implement `NetworkMagic` logic #3775

[CO-410] Implement `NetworkMagic` logic #3775

intricate Oct 24, 2018 •

edited

Loading

intricate Oct 24, 2018 •

edited

Loading