WIP [CDEC-369] Remove HasProtocolConstants in favour of parameters #3133

ruhatch · 2018-06-26T00:42:38Z

Description

Remove the HasProtocolConstants constraint and replace it with explicit parameters. For testing code we use the dummyProtocolConstants defined in Test.Pos.Core.Dummy.

Reviewers please keep a keen eye out for minBound and maxBound changes!

Spent the day rebasing and cleaning this up. Still not ready for review.

Linked issue

CDEC-369

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)

parsonsmatt

This is kinda huge and I see a commit was pushed while I was reviewing. Ping me when you're ready for a review?

parsonsmatt · 2018-06-26T17:06:09Z

auxx/src/Command/Proc.hs

@@ -113,6 +116,7 @@ createCommandProcs mpm hasAuxxMode printAction mDiffusion = rights . fix $ \comm
    },

    let name = "addr-hd" in
+    needsProtocolConstants name >>= \pc ->


is someone afraid of do notation? 🤔

Good point - I might leave this for another PR though, because this whole function is a mess!

parsonsmatt · 2018-06-26T17:07:29Z

auxx/src/Command/Proc.hs

-instance MonadAuxxMode m => ToLeft m Address PublicKey where
-    toLeft = either return makePubKeyAddressAuxx
+-- instance MonadAuxxMode m => ToLeft m Address PublicKey where
+--     toLeft = either return makePubKeyAddressAuxx


Should be deleted?

erikd

Sorry, too many semantically null changes in this as it is. Its already a large PR.

erikd · 2018-06-26T23:14:24Z

auxx/Main.hs


    cArgs@CLI.CommonNodeArgs {..} = aoCommonNodeArgs
    conf = CLI.configurationOptions (CLI.commonArgs cArgs)
-    nArgs =
-        CLI.NodeArgs {behaviorConfigPath = Nothing}
+    nArgs = CLI.NodeArgs {behaviorConfigPath = Nothing}


Up above there was:

sscParams = CLI.gtSscParams cArgs vssSK (npBehaviorConfig nodeParams)

which was wibbled onto two lines and here we have two lines wibbled onto one.

Both changes are just noise with zero semantic change. We should try to keep these semantically null changes out of our PRs.

Hmm - so I was working on the function and it needed formatting both because of my changes and it was poorly laid out before. I know that the change isn't semantic but this seems like the best time to make this kind of change. I'm not going to go through and change the whole codebase, but while I'm already touching a function it seems reasonable to format the whole thing, no?

erikd · 2018-06-26T23:20:52Z

auxx/src/Command/Rollback.hs

    printTipDifficulty = do
        tipDifficulty <- view difficultyL <$> DB.getTipHeader
-        logInfo $ sformat ("Our tip's difficulty is "%build) tipDifficulty
+        logInfo $ sformat ("Our tip's difficulty is " % build) tipDifficulty


I personally don't like the lack of whitespace around the % operator, but Serokell's coding guidelines explicit state that the original version is the right one and the existing code base complies. If we decide to change this it should probably be something enforced by stylish-haskell.

This is fair enough. It's not something that we can enforce using stylish-haskell as far as I'm aware, but I'm happy to revert it if we want to keep it consistent.

erikd · 2018-06-26T23:25:02Z

auxx/src/Command/Tx.hs

+                                            def
+                                            (NE.fromList allAddresses)
+                                            (map TxOutAux outputs)
+                                            curPk


The original line was obvious too long because we can't see the end of the line in the Github diff.

I would probably have formatted that as:

(txAux, _) <- lift $ prepareMTx pm epochSlots getSigner mempty def NE.fromList allAddresses) (map TxOutAux outputs) curPk

which is still under 100 chars wide.

Not sure see the problem here!

erikd · 2018-06-26T23:26:53Z

auxx/src/Command/Tx.hs

-        Right tx -> logInfo $ sformat ("Submitted transaction: "%txaF) tx
+        Left err -> logError
+            $ sformat ("Error: " % stext) (toText $ displayException err)
+        Right tx -> logInfo $ sformat ("Submitted transaction: " % txaF) tx


If this change was simply to get the max line width under 80 chars, i would suggest this is a perfect example of why the line width should stay at 100.

I'm not sure that's a valid argument. The line width wasn't supposed to be 100 anyway, so this is just a change to conform to the style guide. Had the line been incorrectly as 102 when we wanted 100, the fact we have to change it isn't an argument for col width 102!

@ruhatch

Fixing it and taking over this PR because @ruhatch is on leave.

ruhatch · 2018-08-24T20:44:10Z

This has been revived as this PR

ruhatch requested review from adinapoli-iohk, akegalj, denisshevchenko, erikd, ksaric and parsonsmatt as code owners June 26, 2018 00:42

ruhatch force-pushed the ruhatch/CDEC-369 branch 3 times, most recently from 7b630f1 to 325b1a5 Compare June 26, 2018 17:08

parsonsmatt reviewed Jun 26, 2018

View reviewed changes

ruhatch force-pushed the ruhatch/CDEC-369 branch 2 times, most recently from e45a837 to 5bb83d3 Compare June 26, 2018 17:21

erikd previously requested changes Jun 26, 2018

View reviewed changes

ruhatch force-pushed the ruhatch/CDEC-369 branch from 5bb83d3 to e7bfffc Compare June 27, 2018 22:29

ruhatch requested a review from vincenthz as a code owner June 27, 2018 22:29

ruhatch force-pushed the ruhatch/CDEC-369 branch 2 times, most recently from a32025e to da9fc0f Compare June 27, 2018 23:26

erikd force-pushed the ruhatch/CDEC-369 branch from da9fc0f to 9779582 Compare July 1, 2018 20:47

[CDEC-369] Remove HasProtocolConstants in favour of parameters

aa915ee

erikd force-pushed the ruhatch/CDEC-369 branch from 9779582 to aa915ee Compare July 2, 2018 09:52

erikd changed the title ~~[CDEC-369] Remove HasProtocolConstants in favour of parameters~~ WIP [CDEC-369] Remove HasProtocolConstants in favour of parameters Jul 2, 2018

ruhatch mentioned this pull request Aug 24, 2018

[CDEC-369] Remove HasProtocolConstants in favour of parameters #3482

Merged

5 tasks

ruhatch closed this Aug 24, 2018

ruhatch deleted the ruhatch/CDEC-369 branch August 24, 2018 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP [CDEC-369] Remove HasProtocolConstants in favour of parameters #3133

WIP [CDEC-369] Remove HasProtocolConstants in favour of parameters #3133

ruhatch commented Jun 26, 2018 •

edited by erikd

Loading

parsonsmatt left a comment

parsonsmatt Jun 26, 2018

ruhatch Jun 27, 2018

parsonsmatt Jun 26, 2018

erikd left a comment

erikd Jun 26, 2018

ruhatch Jun 27, 2018

erikd Jun 26, 2018

ruhatch Jun 27, 2018

erikd Jun 26, 2018

ruhatch Jun 27, 2018

erikd Jun 26, 2018

ruhatch Jun 27, 2018

ruhatch commented Aug 24, 2018

WIP [CDEC-369] Remove HasProtocolConstants in favour of parameters #3133

WIP [CDEC-369] Remove HasProtocolConstants in favour of parameters #3133

Conversation

ruhatch commented Jun 26, 2018 • edited by erikd Loading

Description

Linked issue

Type of change

Developer checklist

Testing checklist

QA Steps

Screenshots (if available)

parsonsmatt left a 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

erikd left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruhatch commented Aug 24, 2018

ruhatch commented Jun 26, 2018 •

edited by erikd

Loading