IntersectMBO / cardano-db-sync

A component that follows the Cardano chain and stores blocks and transactions in PostgreSQL
Apache License 2.0
293 stars 162 forks source link

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

Open Unisay opened 1 year ago

Unisay commented 1 year ago

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.

sgillespie commented 1 year ago

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

erikd commented 1 year ago

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.

Unisay commented 1 year ago

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);