Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
Unisay opened this issue Sep 29, 2023 · 3 comments · May be fixed by #1524
Open

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

Unisay opened this issue Sep 29, 2023 · 3 comments · May be fixed by #1524
Labels
bug Something isn't working

Comments

@Unisay
Copy link

Unisay commented Sep 29, 2023

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 Unisay added the bug Something isn't working label Sep 29, 2023
@Unisay Unisay linked a pull request Sep 29, 2023 that will close this issue
Open
4 tasks
@sgillespie
Copy link
Contributor

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
Copy link
Contributor

erikd commented Nov 6, 2023

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
Copy link
Author

Unisay commented Nov 6, 2023 •
edited

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
3 participants
@sgillespie @erikd @Unisay