Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

[CO-410] Implement NetworkMagic logic #3775

Merged
merged 1 commit into from
Oct 25, 2018
Merged

[CO-410] Implement NetworkMagic logic #3775

merged 1 commit into from
Oct 25, 2018

Conversation

intricate
Copy link
Contributor

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

@intricate intricate mentioned this pull request Oct 23, 2018
Merged
7 tasks
Jimbo4350
Jimbo4350 reviewed Oct 23, 2018
View reviewed changes
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tools/src/Pos/Tools/Dbgen/Lib.hs Outdated
@@ -248,16 +249,16 @@ generateWalletDB CLI{..} spec@GenSpec{..} = do

case addTo of
Just accId ->
if checkIfAddTo fakeUtxoSpec fakeTxs then
addAddressesTo spec accId
if (checkIfAddTo fakeUtxoSpec fakeTxs) then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant brackets here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@intricate intricate force-pushed the i+j/develop/CO-410/implement-networkmagic branch from 1204e85 to 39dfb58 Compare October 24, 2018 00:10
@intricate intricate removed the wip label Oct 24, 2018
@intricate 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 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 intricate changed the title [DO NOT MERGE] [CO-410] Implement NetworkMagic logic [CO-410] Implement NetworkMagic logic Oct 24, 2018
@intricate intricate force-pushed the i+j/develop/CO-410/implement-networkmagic branch from fdf3427 to 24aec7f Compare October 24, 2018 02:15
erikd
erikd approved these changes Oct 24, 2018
View reviewed changes
Copy link
Member

@erikd erikd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mhuesch
mhuesch approved these changes Oct 24, 2018
View reviewed changes
Copy link
Contributor

@mhuesch mhuesch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :shipit:

tools/src/addr-convert/Main.hs Show resolved Hide resolved
wallet-new/test/unit/Wallet/Inductive/Cardano.hs Outdated
nm <- asks (makeNetworkMagic . ccMagic . tcCardano)
let newRootId = HD.eskToHdRootId nm esk
-- TODO @intricate: Get from TransCtxt or PassiveWallet?
-- nm <- asks (makeNetworkMagic . ccMagic . tcCardano)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Thanks.

wallet/test/Test/Pos/Wallet/Web/Methods/PaymentSpec.hs Show resolved Hide resolved
wallet/test/Test/Pos/Wallet/Web/Tracking/SyncSpec.hs Show resolved Hide resolved
@intricate intricate force-pushed the i+j/develop/CO-410/implement-networkmagic branch from 0e90771 to 7a38603 Compare October 24, 2018 14:11
parsonsmatt
parsonsmatt approved these changes Oct 24, 2018
View reviewed changes
db/src/Pos/DB/Block/Slog/Logic.hs Outdated
@@ -238,7 +240,8 @@ slogApplyBlocks k (ShouldCallBListener callBListener) blunds = do
-- If the program is interrupted at this point (after putting blunds
-- 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@intricate intricate Oct 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake.

lib/src/Pos/Util/UserSecret.hs Outdated
instance Buildable WalletUserSecret where
build WalletUserSecret{..} =
bprint ("{ root = "%addressF%", set name = "%build%
", wallets = "%pairsF%", accounts = "%pairsF%" }")
(makeRootPubKeyAddress fixedNM $ encToPublic _wusRootKey)
(makeRootPubKeyAddress NetworkMainOrStage $ encToPublic _wusRootKey)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@intricate intricate Oct 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tools/src/addr-convert/Main.hs Show resolved Hide resolved
tools/src/addr-convert/Main.hs Show resolved Hide resolved
@intricate 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
@intricate @Jimbo4350
[CO-410] Implement NetworkMagic
80e3d99 
Co-authored-by: Michael Hueschen <[email protected]>
Co-authored-by: Jordan Millar <[email protected]>
@intricate intricate force-pushed the i+j/develop/CO-410/implement-networkmagic branch from e5722ad to 80e3d99 Compare October 25, 2018 14:18
@intricate intricate merged commit 2160095 into develop Oct 25, 2018
@intricate intricate deleted the i+j/develop/CO-410/implement-networkmagic branch October 25, 2018 14:47
@intricate intricate mentioned this pull request Nov 2, 2018
Merged
11 tasks
KtorZ pushed a commit that referenced this pull request Nov 9, 2018
@intricate
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
@intricate
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 intricate mentioned this pull request Mar 22, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Jimbo4350 Jimbo4350 Jimbo4350 left review comments

@parsonsmatt parsonsmatt parsonsmatt approved these changes

@mhuesch mhuesch mhuesch approved these changes

@erikd erikd erikd approved these changes

@ruhatch ruhatch Awaiting requested review from ruhatch

@disassembler disassembler Awaiting requested review from disassembler

Assignees

@Jimbo4350 Jimbo4350

@intricate intricate

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@intricate @erikd @mhuesch @parsonsmatt @Jimbo4350