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

wallet/owner_api: allow owner API port to be configurable #2475

Merged
merged 1 commit into from
Jan 28, 2019

Conversation

RaghavSood
Copy link
Contributor

@RaghavSood RaghavSood commented Jan 27, 2019

Adds an owner_api_listen_port to the grin-wallet.toml config to allow the port for the owner API to be changed on a per-wallet basis.

Related issues/PRs:

#2328, #2405

@@ -72,6 +74,7 @@ impl Default for WalletConfig {
chain_type: Some(ChainTypes::Floonet),
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to your changes but shouldn't this be ChainTypes::Mainnet?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it should, yes.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch! Fixed in #2478

@RaghavSood
Copy link
Contributor Author

I suspected that too but I wasn't sure if there was a reason for it so I left it as-is. I think there's a few floonet remnants floating around the code, and will likely do another pr to resolve those.

@@ -72,6 +74,7 @@ impl Default for WalletConfig {
chain_type: Some(ChainTypes::Floonet),
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it should, yes.

@sesam
Copy link
Contributor

sesam commented Jan 28, 2019

@ignopeverell Anything still blocking merging this?
It is a feature that many have asked for. Would be nice if we can ensure it also gets listed in the release notes.

Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

This looks good.

It does get more awkward to run a wallet for floonet now. We have to remember to explicitly set the port correctly (there is no alternative floonet default config).

In the node global config we override mainnet global config with floonet specific defaults -

grin/config/src/config.rs

Lines 216 to 218 in 92cbfa2

match *chain_type {
global::ChainTypes::Mainnet => {}
global::ChainTypes::Floonet => {

We should be able to do something similar in the wallet config, but I have not looked too closely.

I think this PR is worth merging now. Anybody using floonet can update their local floonet config for now.

Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

Lets merge this and we can work on the floonet defaults in a separate PR.

@antiochp antiochp merged commit f2b6100 into mimblewimble:master Jan 28, 2019
@garyyu
Copy link
Contributor

garyyu commented Jan 28, 2019

Mike Dallas @mcdallas 22:02
#2475 needs to handle the missing value case otherwise everyone's client will break with the next release

Gary Yu @garyyu 22:09
@mcdallas indeed! and need to add the default config file item.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants