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

feat: change emily config to uri #961

Merged
merged 2 commits into from
Nov 28, 2024
Merged

Conversation

cylewitruk
Copy link
Member

Description

Changes the Emily endpoint config to a URI in the format http(s)://[api-key@]<host>:<port>.

Making this change primarily due to the fact that things aren't working well with environment variables and the URI format already supports authentication information.

Changes

  • Modified the emily endpoint format in config
  • Updated tests
  • Updated demo-cli
  • Added the env vars to the local signer .env files 1-3.

Checklist:

  • I have performed a self-review of my code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Copy link
Collaborator

@aldur aldur left a comment

Choose a reason for hiding this comment

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

Much better! Semantically it might be a bit confusing because it's an API key and we are shoving it under the username syntax of the URI, but it makes things a lot simpler, so I am happy with it.

@cylewitruk
Copy link
Member Author

Much better! Semantically it might be a bit confusing because it's an API key and we are shoving it under the username syntax of the URI, but it makes things a lot simpler, so I am happy with it.

Yeah, I figured this was at least more natural than a query param or messing with the url and it's consistent with how we specify other endpoints in the config, so I chose the lesser evil 😊

@cylewitruk cylewitruk self-assigned this Nov 26, 2024
@cylewitruk cylewitruk added the sbtc signer binary The sBTC Bootstrap Signer. label Nov 26, 2024
Copy link
Collaborator

@matteojug matteojug left a comment

Choose a reason for hiding this comment

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

LGTM! Should we mark "breaking changes" PRs (in this case, config related one) somehow? So that we remember there's thing we need to do when updating signers

@cylewitruk
Copy link
Member Author

LGTM! Should we mark "breaking changes" PRs (in this case, config related one) somehow? So that we remember there's thing we need to do when updating signers

Yeah, I brought that up this morning before I read this in the sbtc channel with my fears of protocol changes, I suggested that we have an additional section in the PR template to annotate additional release considerations -- but we should probably have a label as well so that we can easily check all merged PRs before release. I wonder if we can also get the automated release notes to add a "Breaking Changes" section somehow...

@cylewitruk cylewitruk merged commit c658f06 into main Nov 28, 2024
4 checks passed
@cylewitruk cylewitruk deleted the feat/emily-client-config-to-uri branch November 28, 2024 11:08
@cylewitruk cylewitruk added the breaking-local Breaking changes for the local signer (i.e. config, db) label Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-local Breaking changes for the local signer (i.e. config, db) sbtc signer binary The sBTC Bootstrap Signer.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants