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

launch: merge necessary jormungandr config options with user's config #850

Closed
wants to merge 1 commit into from
Closed

launch: merge necessary jormungandr config options with user's config #850

wants to merge 1 commit into from

Conversation

rvl
Copy link
Contributor

@rvl rvl commented Oct 17, 2019

Relates to #832 and #848.

Overview

I think this is the simplest way to permit full jormungandr configuration by the user but still let us control the REST API port.

Comments

I needed to start using lenses for the jormungandr config type.

@rvl rvl self-assigned this Oct 17, 2019
KtorZ
KtorZ reviewed Oct 17, 2019
View reviewed changes
lib/jormungandr/test/unit/Cardano/Wallet/Jormungandr/CompatibilitySpec.hs
}|]

genConfigFile stateDir 8081 baseUrl user `shouldBe` expected
Copy link
Member

Choose a reason for hiding this comment

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

The override aspect isn't really being tested here. It'd be nice to add another test case where the user provide a value for rest.

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Compatibility.hs
override = object
[ "rest" .= object
[ "listen" .= String listen ]
]
Copy link
Member

@KtorZ KtorZ Oct 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Although silently overriding is convenient, I don't think we should be doing anything silently as it might become hard to debug for users. Especially because the config is never printed in the logs (so we never know what's being used in the end).

What about returning a proper error if the rest field was present in the config something like:

"I can't use the given config file because I'd need to override some existing values. Please remove the 'rest' field and let me generate one for you instead."

Also, on a different note: do we need the public_address if we're running a passive node 🤔 ?

@KtorZ KtorZ mentioned this pull request Oct 17, 2019
Closed
1 task
@rvl
Copy link
Contributor Author

rvl commented Oct 18, 2019

There is an undocumented jormungandr --rest-api-listen option we can use, instead of writing the config file.
We will no longer allocate an "available" port for the P2P protocol.
So closing this.

@rvl rvl closed this Oct 18, 2019
@rvl rvl mentioned this pull request Oct 18, 2019
Merged
iohk-bors bot added a commit that referenced this pull request Oct 21, 2019
@iohk-bors @rvl @KtorZ
Merge #862
cfe03b0 
862: jormungandr launch: use --rest-listen instead of generating config.yaml r=KtorZ a=rvl

Relates to #832 and #848.
Supersedes #850.

# Overview

`cardano-wallet-jormungandr launch` specifies the REST API port and storage directory. The user provides the rest of the Jörmungandr configuration (e.g. trusted peers). 

# Comments

The Jörmungandr P2P listen address, port, and log level are no longer configured by cardano-wallet.


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this pull request Oct 21, 2019
@iohk-bors @rvl @KtorZ
Merge #862
fc07f2a 
862: jormungandr launch: use --rest-listen instead of generating config.yaml r=KtorZ a=rvl

Relates to #832 and #848.
Supersedes #850.

# Overview

`cardano-wallet-jormungandr launch` specifies the REST API port and storage directory. The user provides the rest of the Jörmungandr configuration (e.g. trusted peers). 

# Comments

The Jörmungandr P2P listen address, port, and log level are no longer configured by cardano-wallet.


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this pull request Oct 22, 2019
@iohk-bors @rvl @KtorZ
Merge #862
b001b5e 
862: jormungandr launch: use --rest-listen instead of generating config.yaml r=rvl a=rvl

Relates to #832 and #848.
Supersedes #850.

# Overview

`cardano-wallet-jormungandr launch` specifies the REST API port and storage directory. The user provides the rest of the Jörmungandr configuration (e.g. trusted peers). 

# Comments

The Jörmungandr P2P listen address, port, and log level are no longer configured by cardano-wallet.


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this pull request Oct 22, 2019
@iohk-bors @rvl @KtorZ
Merge #862 #864
fe5e92d 
862: jormungandr launch: use --rest-listen instead of generating config.yaml r=KtorZ a=rvl

Relates to #832 and #848.
Supersedes #850.

# Overview

`cardano-wallet-jormungandr launch` specifies the REST API port and storage directory. The user provides the rest of the Jörmungandr configuration (e.g. trusted peers). 

# Comments

The Jörmungandr P2P listen address, port, and log level are no longer configured by cardano-wallet.


864: nix: Provide derivations for Daedalus installer r=KtorZ a=rvl

Relates to #863.
Based on #828.

# Overview

- @disassembler @cleverca22 It's not exactly the same as before but should work ok I think.
- Adds source filtering to avoid unnecessary rebuilds.

# Comments

To build:

```
nix-build -A cardano-wallet-jormungandr
nix-build release.nix -A daedalus-jormungandr.windows -o daedalus-jormungandr-windows
nix-build release.nix -A daedalus-jormungandr.macos -o daedalus-jormungandr-macos
nix-build release.nix -A daedalus-jormungandr.linux -o daedalus-jormungandr-linux
```

Note that `daedalus-jormungandr.{windows,macos,linux}` from `release.nix` reference the same `cardano-wallet-jormungandr` derivation, only with different `system` or `crossSystem` arguments. So Daedalus may also import from `default.nix` rather than `release.nix`.

Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this pull request Oct 22, 2019
@iohk-bors @rvl
@KtorZ @paweljakubas
Merge #862 #864 #878 #879
df16a27 
862: jormungandr launch: use --rest-listen instead of generating config.yaml r=KtorZ a=rvl

Relates to #832 and #848.
Supersedes #850.

# Overview

`cardano-wallet-jormungandr launch` specifies the REST API port and storage directory. The user provides the rest of the Jörmungandr configuration (e.g. trusted peers). 

# Comments

The Jörmungandr P2P listen address, port, and log level are no longer configured by cardano-wallet.


864: nix: Provide derivations for Daedalus installer r=KtorZ a=rvl

Relates to #863.
Based on #828.

# Overview

- @disassembler @cleverca22 It's not exactly the same as before but should work ok I think.
- Adds source filtering to avoid unnecessary rebuilds.

# Comments

To build:

```
nix-build -A cardano-wallet-jormungandr
nix-build release.nix -A daedalus-jormungandr.windows -o daedalus-jormungandr-windows
nix-build release.nix -A daedalus-jormungandr.macos -o daedalus-jormungandr-macos
nix-build release.nix -A daedalus-jormungandr.linux -o daedalus-jormungandr-linux
```

Note that `daedalus-jormungandr.{windows,macos,linux}` from `release.nix` reference the same `cardano-wallet-jormungandr` derivation, only with different `system` or `crossSystem` arguments. So Daedalus may also import from `default.nix` rather than `release.nix`.

878: Support for legacy UTxO witness in Jörmungandr r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#779 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have extended `mkStdTx` so that it would properly handle transactions coming from a `RndKey` and construct `legacy-utxo` witnesses for it.

# Comments

<!-- Additional comments or screenshots to attach if any -->

Jörmungandr doesn't implement creating legacy utxo witnesses from jcli ... 

[jcli/src/jcli_app/transaction/mk_witness.rs](https://github.com/input-output-hk/jormungandr/blob/master/jcli/src/jcli_app/transaction/mk_witness.rs#L78-L82)
```rust
            WitnessType::OldUTxO => {
                // TODO unimplemented!()
                let _secret_key: SecretKey<Ed25519Bip32> = self.secret()?;
                Err(Error::MakeWitnessLegacyUtxoUnsupported)?;
                unimplemented!()
            }
``` 

so I had to construct them by hand according to:

- The format defined in [chain-impl-mockchain#witnesses](https://github.com/input-output-hk/chain-libs/blob/incentive/chain-impl-mockchain/doc/format.md#witnesses)

- The format from the source (double checking it matches the doc..) [chain-impl-mockchain/src/transaction/witness.rs](https://github.com/input-output-hk/chain-libs/blob/incentive/chain-impl-mockchain/src/transaction/witness.rs#L173-L176)

It'd be nice to review our golden if / when the constructor for the witness type gets updated.

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


879: Porting the rest of forget pending tx integration tests to CLI r=KtorZ a=paweljakubas

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->
#836 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have ported the rest of forgetting pending tx integration tests to CLI

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io>
iohk-bors bot added a commit that referenced this pull request Oct 22, 2019
@iohk-bors @rvl
@KtorZ @paweljakubas
Merge #862 #864 #878 #879
3d10b96 
862: jormungandr launch: use --rest-listen instead of generating config.yaml r=KtorZ a=rvl

Relates to #832 and #848.
Supersedes #850.

# Overview

`cardano-wallet-jormungandr launch` specifies the REST API port and storage directory. The user provides the rest of the Jörmungandr configuration (e.g. trusted peers). 

# Comments

The Jörmungandr P2P listen address, port, and log level are no longer configured by cardano-wallet.


864: nix: Provide derivations for Daedalus installer r=KtorZ a=rvl

Relates to #863.
Based on #828.

# Overview

- @disassembler @cleverca22 It's not exactly the same as before but should work ok I think.
- Adds source filtering to avoid unnecessary rebuilds.

# Comments

To build:

```
nix-build -A cardano-wallet-jormungandr
nix-build release.nix -A daedalus-jormungandr.windows -o daedalus-jormungandr-windows
nix-build release.nix -A daedalus-jormungandr.macos -o daedalus-jormungandr-macos
nix-build release.nix -A daedalus-jormungandr.linux -o daedalus-jormungandr-linux
```

Note that `daedalus-jormungandr.{windows,macos,linux}` from `release.nix` reference the same `cardano-wallet-jormungandr` derivation, only with different `system` or `crossSystem` arguments. So Daedalus may also import from `default.nix` rather than `release.nix`.

878: Support for legacy UTxO witness in Jörmungandr r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#779 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have extended `mkStdTx` so that it would properly handle transactions coming from a `RndKey` and construct `legacy-utxo` witnesses for it.

# Comments

<!-- Additional comments or screenshots to attach if any -->

Jörmungandr doesn't implement creating legacy utxo witnesses from jcli ... 

[jcli/src/jcli_app/transaction/mk_witness.rs](https://github.com/input-output-hk/jormungandr/blob/master/jcli/src/jcli_app/transaction/mk_witness.rs#L78-L82)
```rust
            WitnessType::OldUTxO => {
                // TODO unimplemented!()
                let _secret_key: SecretKey<Ed25519Bip32> = self.secret()?;
                Err(Error::MakeWitnessLegacyUtxoUnsupported)?;
                unimplemented!()
            }
``` 

so I had to construct them by hand according to:

- The format defined in [chain-impl-mockchain#witnesses](https://github.com/input-output-hk/chain-libs/blob/incentive/chain-impl-mockchain/doc/format.md#witnesses)

- The format from the source (double checking it matches the doc..) [chain-impl-mockchain/src/transaction/witness.rs](https://github.com/input-output-hk/chain-libs/blob/incentive/chain-impl-mockchain/src/transaction/witness.rs#L173-L176)

It'd be nice to review our golden if / when the constructor for the witness type gets updated.

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


879: Porting the rest of forget pending tx integration tests to CLI r=KtorZ a=paweljakubas

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->
#836 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have ported the rest of forgetting pending tx integration tests to CLI

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io>
iohk-bors bot added a commit that referenced this pull request Oct 22, 2019
@iohk-bors @rvl @KtorZ
Merge #862 #864
6ba5bfd 
862: jormungandr launch: use --rest-listen instead of generating config.yaml r=KtorZ a=rvl

Relates to #832 and #848.
Supersedes #850.

# Overview

`cardano-wallet-jormungandr launch` specifies the REST API port and storage directory. The user provides the rest of the Jörmungandr configuration (e.g. trusted peers). 

# Comments

The Jörmungandr P2P listen address, port, and log level are no longer configured by cardano-wallet.


864: nix: Provide derivations for Daedalus installer r=KtorZ a=rvl

Relates to #863.
Based on #828.

# Overview

- @disassembler @cleverca22 It's not exactly the same as before but should work ok I think.
- Adds source filtering to avoid unnecessary rebuilds.

# Comments

To build:

```
nix-build -A cardano-wallet-jormungandr
nix-build release.nix -A daedalus-jormungandr.windows -o daedalus-jormungandr-windows
nix-build release.nix -A daedalus-jormungandr.macos -o daedalus-jormungandr-macos
nix-build release.nix -A daedalus-jormungandr.linux -o daedalus-jormungandr-linux
```

Note that `daedalus-jormungandr.{windows,macos,linux}` from `release.nix` reference the same `cardano-wallet-jormungandr` derivation, only with different `system` or `crossSystem` arguments. So Daedalus may also import from `default.nix` rather than `release.nix`.

Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this pull request Oct 22, 2019
@iohk-bors @rvl @KtorZ
Merge #862
5231e58 
862: jormungandr launch: use --rest-listen instead of generating config.yaml r=KtorZ a=rvl

Relates to #832 and #848.
Supersedes #850.

# Overview

`cardano-wallet-jormungandr launch` specifies the REST API port and storage directory. The user provides the rest of the Jörmungandr configuration (e.g. trusted peers). 

# Comments

The Jörmungandr P2P listen address, port, and log level are no longer configured by cardano-wallet.


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
@rvl rvl deleted the rvl/848/jm-launch-config branch January 22, 2020 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KtorZ KtorZ KtorZ left review comments

Assignees

@rvl rvl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rvl @KtorZ