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

Scaffold lib/jormungandr folder #244

Merged
merged 3 commits into from
May 9, 2019
Merged

Scaffold lib/jormungandr folder #244

merged 3 commits into from
May 9, 2019

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented May 8, 2019 •
edited
Loading

Issue Number

#218

Overview

  • I added lib/jormungandr based upon lib/http-bridge
  • Most notably there is {Binary,Compatibility,Transaction}.Jormungandr.hs, and Cardano.Wallet.Binary.JormungandrSpec

Comments

  • Ah 🤦‍♂ I think the new format is not even CBOR. I have now started from a cleaner slate than initially.

@Anviking Anviking self-assigned this May 8, 2019
@Anviking Anviking force-pushed the anviking/218/jörmungandr-blocks branch from 954f44f to 7dae32b Compare May 8, 2019 17:45
@Anviking Anviking changed the title Scaffold lib/jormungandr folder [WIP] Scaffold lib/jormungandr folder May 8, 2019
@Anviking Anviking force-pushed the anviking/218/jörmungandr-blocks branch from 31d0c2e to df0948e Compare May 8, 2019 20:13
@Anviking Anviking changed the title [WIP] Scaffold lib/jormungandr folder Scaffold lib/jormungandr folder May 8, 2019
Anviking
Anviking commented May 8, 2019
View reviewed changes
lib/jormungandr/src/Cardano/Environment.hs Outdated
-- rely on a `.env` file to bundle configuration settings together for a given
-- target environment.

module Cardano.Environment
Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure moving lib/{jormungandr, http-bridge}/Cardano/Environment.hs to cardano-wallet-core was desired, so I kept the duplication for now.

Copy link
Member

Choose a reason for hiding this comment

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

Well, network discrimination is different on Jörmungandr, and probably will be too with Shelley Haskell. There are actually only too network (testnet or mainnet) and it's encoded on a single byte (Cf the new address format). Perhaps it'd make sense to rename both modules to Cardano.Environment.{Jormungandr,HttpBridge}.

Copy link
Member

Choose a reason for hiding this comment

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

Yet, we could factor out in core the "Internal" bits with the ErrMissingOrInvalidEnvVar and unsafeLookupEnv since this is fairly network-agnostic.

@Anviking Anviking requested a review from KtorZ May 8, 2019 20:49
@KtorZ KtorZ mentioned this pull request May 8, 2019
Closed
5 tasks
@KtorZ
Copy link
Member

KtorZ commented May 9, 2019

I think the new format is not even CBOR. I have now started from a cleaner slate than initially.

Indeed 🙃

KtorZ
KtorZ approved these changes May 9, 2019
View reviewed changes
Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

Thanks for this. I think we can factor our some of the logic in Cardano.Environment (to core), and rename both modules in the bridge and jormungandr to implement the specific bits.

@Anviking Anviking force-pushed the anviking/218/jörmungandr-blocks branch 4 times, most recently from 86ff6b7 to fba0951 Compare May 9, 2019 20:32
@Anviking Anviking force-pushed the anviking/218/jörmungandr-blocks branch 2 times, most recently from ddd0b50 to 693e13e Compare May 9, 2019 21:15
@Anviking
Factor out unsafeLookupEnv
588d11e 
- Added System.Environment.Lookup and System.Environment.LookupSpec to
core

- Removed common parts from
    - Cardano.Environment.HttpBridge
    - Cardano.Environment.HttpBridgeSpec
    - Cardano.Environment.Jormungandr
    - Cardano.Environment.JormungandrSpec (added)
@Anviking Anviking force-pushed the anviking/218/jörmungandr-blocks branch from 693e13e to 588d11e Compare May 9, 2019 21:32
@Anviking Anviking merged commit 1b12c1d into master May 9, 2019
@KtorZ KtorZ deleted the anviking/218/jörmungandr-blocks branch May 16, 2019 09:58
akegalj
akegalj reviewed May 22, 2019
View reviewed changes
Copy link
Contributor

@akegalj akegalj left a comment

Choose a reason for hiding this comment

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

lgtm

lib/jormungandr/src/Cardano/Environment/Jormungandr.hs
-- configuration data at runtime using the available environment.
--
-- This gives us a flexible and portable approach to software configuration, and
-- remove some pain from the development perspective. Prior to starting, the
Copy link
Contributor

Choose a reason for hiding this comment

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

s/remove/removes

plural

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akegalj akegalj akegalj left review comments

@KtorZ KtorZ KtorZ approved these changes

Assignees

@Anviking Anviking

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Anviking @KtorZ @akegalj