cardano-db-sync icon indicating copy to clipboard operation
cardano-db-sync copied to clipboard

A typeclass instance in scope of the Cardano.DbSync.Sync

Open Unisay opened this issue 2 years ago • 3 comments

OS not relevant

Versions latest master

Build/Install Method not relevant

Run method not relevant

Additional context https://www.haskell.org/onlinereport/haskell2010/haskellch5.html#x11-1060005.4

Instance declarations cannot be explicitly named on import or export lists. All instances in scope within a module are always exported and any import brings all instances in from the imported module. Thus, an instance declaration is in scope if and only if a chain of import declarations leads to the module containing the instance declaration.

Problem Report

At the moment Cardano.DbSync.Sync depends on the typeclass instance TranslateProto (TPraos c1) (Praos c2) and wouldn't compile without it in scope. In particular, these expressions require it:

supportedNodeToClientVersions (Proxy @CardanoBlock)
consensusErrorPolicy (Proxy @CardanoBlock)

it is very hard to find out that such an instance is brought by via the Cardano.DbSync.Util:

Cardano.DbSync.Sync imports Cardano.DbSync.Util imports Ouroboros.Consensus.Protocol.Praos.Translate

the problem with this is that its not intuitive and if in some imaginary future Cardano.DbSync.Sync doesn't import the Cardano.DbSync.Util anymore, then a compilation error arises:

    • No instance for (Ouroboros.Consensus.Protocol.Translate.TranslateProto
                         (TPraos StandardCrypto) (Praos StandardCrypto))
        arising from a use of ‘supportedNodeToClientVersions’

I propose to import Ouroboros.Consensus.Protocol.Praos.Translate directly where its required thus making a code more explicit and easier to reason about.

Unisay avatar Sep 29 '23 16:09 Unisay

I propose to import Ouroboros.Consensus.Protocol.Praos.Translate directly where its required thus making a code more explicit and easier to reason about.

I tend to agree. Chasing down orphan instances can be very frustrating

sgillespie avatar Oct 09 '23 13:10 sgillespie

Would it not be better to get the needed type class added to ouroborous-consensus ?

That would mean no orphan instances and possible instance clash of if gets added to ouroborous-consensus some time after db-sync implements its own orphan.

erikd avatar Nov 06 '23 05:11 erikd

I don't quite understand this suggestion:

Would it not be better to get the needed type class added to ouroborous-consensus ?

If you're referring to the TranslateProto class then it already is in the ouroboros-consensus.

That would mean no orphan instances and possible instance clash of if gets added to ouroborous-consensus some time after db-sync implements its own orphan.

This particular issue is more about the way the existing typeclass instance is brought into scope: explicit (direct import) vs. implicit (via another import) , (its not about orphan / non-orphan nature of such an instance);

Unisay avatar Nov 06 '23 12:11 Unisay