-
Notifications
You must be signed in to change notification settings - Fork 632
[CDEC-418] Reunite orphan Bi instances in txp #3179
Conversation
@@ -65,6 +66,10 @@ data DataMsg contents = DataMsg | |||
{ dmContents :: !contents | |||
} deriving (Generic, Show, Eq) | |||
|
|||
instance Bi (DataMsg TxMsgContents) where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be in txp/src/Pos/Txp/Network/Types.hs
with TxMsgContents
as per: https://iohk.myjetbrains.com/youtrack/issue/CDEC-401 @erikd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally probably yes, and in the end, maybe yes, but right now, definitely no :).
What we are aiming for at the moment is to fix txp
so it no longer depends in infra
and in the end, make infra
depend on txp
. We also want to avoid doing this in one huge PR that takes a week or more to get ready and then days to review etc.
So, we are temporarily going to contravene our normal good practices and do what ever it takes to fix the way txp
and infra
depend on each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting on Erik's feedback
ee38f0f
to
7944820
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Needs a rebase and would be good to fix the stylish haskell wibbles along the way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
7944820
to
10fda77
Compare
10fda77
to
369bf54
Compare
I'm going to merge this as soon as it passes CI :) |
Description
This PR reunites orphan Bi instances from
txp/src/Pos/Txp/Binary*
with their type definitions.Notes
It turned out that moving the
Bi
instance for(DataMsg TxMsgContents)
was a lot more involved than I had anticipated.Since
DataMsg
is a type defined ininfra
rather thantxp
, I decided that theBi
instance was best suited to be ininfra
. However, if I were to do this,infra
would then have to depend upontxp
sinceTxMsgContents
is a type defined inPos.Txp.Network.Types
. This would introduce a circular dependency sincetxp
already depends uponinfra
.As a result of this, I ended up moving
TxMsgContents
fromtxp
toinfra
.I also moved all of the
Arbitrary
instances fromTest.Pos.Txp.Arbitrary
(txp-test
) toTest.Pos.Core.Arbitrary.Txp
(core-test
) since they were all instances ofcore
types anyways.Linked issue
https://iohk.myjetbrains.com/youtrack/issue/CDEC-418
Type of change
Developer checklist
Testing checklist
QA Steps
Screenshots (if available)