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.

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

Factor out class ShelleyBasedBlock and type family ShelleyBlockLedgerEra #1073

Merged
merged 3 commits into from
Apr 24, 2024

Conversation

dnadales
Copy link
Member

Pending change from #934 (comment)

@dnadales dnadales requested a review from a team as a code owner April 23, 2024 12:12
Comment on lines 143 to 150
class
( ShelleyBasedEra (ShelleyBlockLedgerEra blk)
, blk ~ ShelleyBlock (BlockProtocol blk) (ShelleyBlockLedgerEra blk)
) => ShelleyBasedBlock blk

instance ( proto ~ BlockProtocol (ShelleyBlock proto era)
, ShelleyBasedEra era
) => ShelleyBasedBlock (ShelleyBlock proto era)
Copy link
Member

@amesgen amesgen Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could give this class "more power", ie make its superclass constraints stronger, eg use ShelleyCompatible instead of just ShelleyBasedEra:

Suggested change
class
( ShelleyBasedEra (ShelleyBlockLedgerEra blk)
, blk ~ ShelleyBlock (BlockProtocol blk) (ShelleyBlockLedgerEra blk)
) => ShelleyBasedBlock blk
instance ( proto ~ BlockProtocol (ShelleyBlock proto era)
, ShelleyBasedEra era
) => ShelleyBasedBlock (ShelleyBlock proto era)
class
( ShelleyCompatible (BlockProtocol blk) (ShelleyBlockLedgerEra blk)
, blk ~ ShelleyBlock (BlockProtocol blk) (ShelleyBlockLedgerEra blk)
) => ShelleyBasedBlock blk
instance ( proto ~ BlockProtocol (ShelleyBlock proto era)
, ShelleyCompatible proto era
) => ShelleyBasedBlock (ShelleyBlock proto era)

This might come in handy in a future use case where we want to call a function that requires ShelleyCompatible and not just ShelleyBasedEra in an SOP combinator.

Copy link
Contributor

@jasagredo jasagredo Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also proposed the same. I do wonder if this is not equivalent to requiring it in the constructors? as in:

data ShelleyBlock proto era where
  ShelleyBlock :: ShelleyCompatible proto era => 
    { ... } -> ShelleyBlock proto era

leaving that aside, it might be intereseting to provide instances for Header (ShelleyBlock ...) and LedgerState (ShelleyBlock ...)? I feel like this might simplify some aux classes we might have around.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one should prefer function constraints, not constraints on constructors. They are not entirely equivalent

Most of the places where I use constraints inside constructors is when dealing with existentials or Dicts

@dnadales dnadales self-assigned this Apr 23, 2024
@dnadales dnadales enabled auto-merge April 24, 2024 13:29
@dnadales dnadales added this pull request to the merge queue Apr 24, 2024
Merged via the queue into main with commit f25ea0b Apr 24, 2024
18 of 21 checks passed
@dnadales dnadales deleted the dnadales/factor-out-shelley-based-block branch April 24, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants