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

Move Hash type and related functions into separate module. #2303

Merged
merged 2 commits into from
Nov 9, 2020

Conversation

jonathanknowles
Copy link
Contributor

@jonathanknowles jonathanknowles commented Nov 9, 2020

Issue Number

Preparation for ADP-347.

Overview

This PR moves the Hash type and related functions to a separate module Cardano.Wallet.Primitive.Types.Hash. This allows code to use the Hash type without having to incur a dependency on the Primitive.Types module, which is already very large and slow to recompile.

Comments

This PR doesn't re-export the Hash type from Primitive.Types.

Although it would be convenient to be able to import everything from Primitive.Types in one go (with a qualified import), it would also create a compilation bottleneck, since anything that imported Hash from the Primitive.Types module would have to depend on everything required to build Primitive.Types.

This PR follows the advice presented in Keeping Compilation Fast by Matt Parsons.

@jonathanknowles jonathanknowles requested a review from rvl November 9, 2020 06:35
@jonathanknowles jonathanknowles self-assigned this Nov 9, 2020
@jonathanknowles jonathanknowles added IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG and removed IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG labels Nov 9, 2020
@rvl rvl added the IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG label Nov 9, 2020
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Seems reasonable to re-use the Hash type for monetary policy ID.

This will fix the circular dependency.

Moving more types to separate modules and not re-exporting from a single types module may help reduce compilation in future.

Looks fine, as long as it builds and passes stylish-haskell.

@jonathanknowles jonathanknowles force-pushed the jonathanknowles/extract-out-hash-type branch from c2aac52 to a0e92b9 Compare November 9, 2020 06:54
This allows code to use the `Hash` type without having to incur a
dependency on the `Primitive.Types` module, which is already very large
and slow to recompile.
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/extract-out-hash-type branch from a0e92b9 to 378930d Compare November 9, 2020 07:06
@jonathanknowles jonathanknowles changed the title Move Hash type and related functions into dedicated module. Move Hash type and related functions into separate module. Nov 9, 2020
@jonathanknowles
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Nov 9, 2020
2303: Move `Hash` type and related functions into separate module. r=jonathanknowles a=jonathanknowles

# Issue Number

Preparation for [ADP-347](https://jira.iohk.io/browse/ADP-347).

# Overview

This PR moves the `Hash` type and related functions to a separate module `Cardano.Wallet.Primitive.Types.Hash`. This allows code to use the `Hash` type without having to incur a dependency on the `Primitive.Types` module, which is already very large and slow to recompile.

# Comments

This PR doesn't re-export the `Hash` type from `Primitive.Types`.

Although it would be convenient to be able to import everything from `Primitive.Types` in one go (with a qualified import), it would also create a compilation bottleneck, since anything that imported `Hash` from the `Primitive.Types` module would have to depend on everything required to build `Primitive.Types`.

This PR follows the advice presented in [Keeping Compilation Fast](https://www.parsonsmatt.org/2019/11/27/keeping_compilation_fast.html) by Matt Parsons.

Co-authored-by: Jonathan Knowles <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 9, 2020

Build failed:

Cardano.Wallet.DB.Sqlite
  Sqlite
    Extra Properties about DB initialization
      createWallet . listWallets yields expected results
        +++ OK, passed 100 tests.
      creating same wallet twice yields an error
        +++ OK, passed 100 tests.
      removing the same wallet twice yields an error
        +++ OK, passed 100 tests.
    put . read yields a result
      Checkpoint
        +++ OK, passed 100 tests.
      Wallet Metadata
        +++ OK, passed 100 tests.
      Tx History
        +++ OK, passed 100 tests.
      Private Key
        +++ OK, passed 100 tests.
    getTx properties
      can read after putting tx history for valid tx id
        +++ OK, passed 100 tests.
      cannot read after putting tx history for invalid tx id
        +++ OK, passed 100 tests.
      cannot read after putting tx history for invalid wallet id
        +++ OK, passed 100 tests; 7 discarded.
    can't put before wallet exists
      Checkpoint
building of '/nix/store/ncly6n9wmkp2s5lpikqzvvpmsa4sh297-cardano-wallet-core-test-unit-2020.11.3-x86_64-unknown-linux-musl-check-x86_64-unknown-linux-musl' timed out after 7200 seconds of silence

#2292

@jonathanknowles
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 9, 2020

Build succeeded:

@iohk-bors iohk-bors bot merged commit c4ac613 into master Nov 9, 2020
@iohk-bors iohk-bors bot deleted the jonathanknowles/extract-out-hash-type branch November 9, 2020 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants