Skip to content

feat: Configuration option to include foreign API endpoints in the owner API#2305

Merged
yeastplume merged 2 commits intomimblewimble:masterfrom
ducembarr:ducembarr-owner-receive
Jan 7, 2019
Merged

feat: Configuration option to include foreign API endpoints in the owner API#2305
yeastplume merged 2 commits intomimblewimble:masterfrom
ducembarr:ducembarr-owner-receive

Conversation

@ducembarr
Copy link
Contributor

@ducembarr ducembarr commented Jan 6, 2019

This pull request adds an owner_api_include_foreign wallet configuration option to make endpoints from the Foreign API available over the same port as the Owner API.

A few notes on the actual change:

  • This feature is useful when running the wallet in environments that make it difficult to access multiple ports on the same container. In particular, Amazon's ECS and the various Amazon load balancers make it very difficult to have multiple ports open on the same container.
  • The feature defaults to false.
  • There shouldn't be any overlapping paths because the Foreign API endpoints start with /v1/wallet/foreign and the Owner API endpoints start with /v1/wallet/owner/.
  • I do not believe there are any security concerns about setting the configuration option, because presumably anyone trusted to access the Owner API (which can perform arbitrary sends) is trusted to access the Foreign API.

I have added a basic integration test to make sure setting the configuration option on/off works as expected. This involved making some changes to the test harness, so I've separated the changes for the tests into a separate commit in case those changes are problematic.

This is my first PR to this repository, so as usual please let me know if I'm missing anything major from the PR or description. Thanks!

@yeastplume
Copy link
Member

Looks good to me, and thanks for submitting this. Appreciate very much that you've also included tests and additions to documentation where necessary. Thanks!

@yeastplume yeastplume merged commit a3431fb into mimblewimble:master Jan 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants