-
Notifications
You must be signed in to change notification settings - Fork 631
[CBR-305] SafeCopy instances for InDb types #3381
Conversation
bs :: B.ByteString <- SC.safeGet | ||
pure (InDb (Core.RedeemPublicKey (Ed25519.PublicKey bs))) | ||
putCopy (InDb (Core.RedeemPublicKey pk)) = SC.contain $ do | ||
SC.safePut (Ed25519.unPublicKey pk :: B.ByteString) |
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.
src/Cardano/Wallet/Kernel/DB/InDb.hs:264:17: warning: [-Wdeprecations]
In the use of ‘unPublicKey’ (imported from Crypto.Sign.Ed25519):
Deprecated: "This accessor is deprecated, and will be removed in a future version. Use @'openPublicKey'@ instead."
|
264 | SC.safePut (Ed25519.unPublicKey pk :: B.ByteString)
| ^^^^^^^^^^^^^^^^^^^
<no location info>: error:
Failing due to -Werror.
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.
Don't worry, this is WIP :)
QuickCheck properties are still missing, but the instances are done. |
Neat, we got test failures 😄 |
|
||
instance SC.SafeCopy (InDb Core.Coin) where | ||
getCopy = SC.contain $ do | ||
w :: Word64 <- SC.safeGet |
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.
Is that a 7 space indent?
|
||
instance SC.SafeCopy (InDb Txp.TxAux) where | ||
getCopy = SC.contain $ do | ||
InDb (tx :: Txp.Tx) <- SC.safeGet |
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.
I also personally prefer 2 space indent, but this code ....
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.
Yeah, I'll get the style right before merging it all in. I'm mostly concerned about the correctness issues and failing tests atm
@@ -1,4 +1,5 @@ | |||
{-# OPTIONS_GHC -fno-warn-orphans #-} |
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.
Why does this need to alllow orphans? I don't immediately see it.
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.
Good catch! We can safely remove that warning :)
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.
🎉
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.
Looking good! Let me take a quick look to see if the rebase would be hard.
putCopy (InDb (AddressMeta isUsed isChange)) = contain $ do | ||
safePut isUsed | ||
safePut isChange | ||
|
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.
This instance is superfluous, InDb
should only be used to tag types defined in core, anything defined by the wallet itself (that we have control ovr) we can just use deriveSafeCopy
.
{------------------------------------------------------------------------------- | ||
Wrap core types so that we can make independent serialization decisions | ||
-------------------------------------------------------------------------------} | ||
|
||
-- | Wrapped type (with potentially different 'SafeCopy' instance) | ||
-- | Wrapped type (with potentially different 'SC' instance) |
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.
Why this renaming?
Thanks for this @parsonsmatt ! As agreed, I'll take it from here, let's get this thing done and merged :) |
Fixed the tests. Am now going to rebase, then force push. |
Also placate hlint, stylish-haskell, stack2nix
I recompiled the {-# OPTIONS_GHC -ddump-ds -ddump-to-file #-} and grepped for
In other words, none from core! Perfect :) |
Merging into |
Muchas gracias :) |
Some things to keep in mind:
Round-trip QuickCheck tests are missing.
The basic approach to defining instances for types wrapped in
InDb
is described in the code:There are a lot of redundant type signatures. This is deliberate, so that changing the definition of a datatype in the future triggers a type-checker error if possible.