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

Change owner API port for MainNet #2405

Merged
merged 2 commits into from Jan 21, 2019
Merged

Change owner API port for MainNet #2405

merged 2 commits into from Jan 21, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jan 17, 2019 •
edited by ghost
Loading

Fixes #2389 ?

antiochp
antiochp reviewed Jan 18, 2019
View reviewed changes
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 will fix for mainnet but should really needs to be configurable via toml file?
Otherwise we're just going to break floonet.

@ghost
Copy link
Author

ghost commented Jan 18, 2019

I could make the port configurable (But not the address, since that looks to be part of a wider discussion around HTTPS and running owner API outside of localhost) which solves this issue for now?

@antiochp
Copy link
Member

Oh I see - normally we configure the whole thing (ip+port). But here we need to limit it to the port.

I think we should do this yes, following the existing config approach as closely as possible but for the port only.

Otherwise we fix it for mainnet and immediately break it for floonet for anyone running both on the same system (and having floonet pointing at hard-coded mainnet port is arguably worse).

@antiochp antiochp added this to the 1.0.1 milestone Jan 18, 2019
@ignopeverell
Copy link
Contributor

Right, so a basic if for floonet to just hardcode it to a different port?

@antiochp
Copy link
Member

Right, so a basic if for floonet to just hardcode it to a different port?

That would be the pragmatic approach for v1.0.1. 👍

@sesam
Copy link
Contributor

sesam commented Jan 20, 2019

@thedevworks Good initiative! Find is_floo in the code and you'll soon be done :)

@ghost
Copy link
Author

ghost commented Jan 21, 2019

Thanks, I've updated to set the port based on is_floonet. I have tested with a fresh node and wallet with and without --floonet parameter, and the port is selected as expected.

antiochp
antiochp approved these changes Jan 21, 2019
View reviewed changes
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.

👍

@antiochp antiochp merged commit 2072e48 into mimblewimble:master Jan 21, 2019
@0xmichalis 0xmichalis mentioned this pull request Jan 21, 2019
Merged
sesam added a commit to sesam/grin that referenced this pull request Jan 21, 2019
@sesam
update ports since PR mimblewimble#2405. We now allow both floo and m…
a1286f2 
…ainnet owner_apis to run in parallell on a single server, with ports 13420 or 3420
@antiochp antiochp mentioned this pull request Jan 22, 2019
Closed
@ignopeverell ignopeverell mentioned this pull request Jan 23, 2019
Closed
antiochp pushed a commit that referenced this pull request Jan 25, 2019
@sesam @antiochp
fix owner_api docs: wrong port number (#2442)
0e0ec24 
* fix owner_api docs: wrong port number

* update ports since PR #2405. We now allow both floo and mainnet owner_apis to run in parallell on a single server, with ports 13420 or 3420

* correction since there is no --mainnet flag

* fixes the comment by @karkagis
@RaghavSood
Copy link
Contributor

RaghavSood commented Jan 26, 2019 •
edited
Loading

I do think that the owner API port should be configurable, and not hard coded to 3420. A very simple usecase we are encountering at an exchange is to be able to run the owner API for two separate wallets (not separate accounts, but entirely separate wallets - different seeds and everything) on the same machine. This is currently not possible with the hard coded ports, and requires that we spin up containers/machines for each wallet, which is not ideal.

I'm happy to make a PR for the change if you wish.

@RaghavSood RaghavSood mentioned this pull request Jan 27, 2019
Merged
bitgrin pushed a commit to bitgrin/bitgrin that referenced this pull request Mar 2, 2019
@pharazen
Change owner API port for MainNet (mimblewimble#2405)
d54e26c 
* Change owner API port for MainNet

* Added owner_api port selector based on is_floonet
bitgrin pushed a commit to bitgrin/bitgrin that referenced this pull request Mar 2, 2019
@pharazen
Change owner API port for MainNet (mimblewimble#2405)
3905f07 
* Change owner API port for MainNet

* Added owner_api port selector based on is_floonet
bitgrin pushed a commit to bitgrin/bitgrin that referenced this pull request Mar 2, 2019
@sesam @pharazen
fix owner_api docs: wrong port number (mimblewimble#2442)
b02b240 
* fix owner_api docs: wrong port number

* update ports since PR mimblewimble#2405. We now allow both floo and mainnet owner_apis to run in parallell on a single server, with ports 13420 or 3420

* correction since there is no --mainnet flag

* fixes the comment by @karkagis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0.1
Development

Successfully merging this pull request may close these issues.
4 participants
@antiochp @ignopeverell @sesam @RaghavSood