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

fix: allow loop creds to be empty if not enabled #1891

Merged
merged 5 commits into from
Oct 28, 2022
Merged

Conversation

openoms
Copy link
Collaborator

@openoms openoms commented Oct 27, 2022

moved from #1733

throw new ConfigError("Missing LND1_LOOP_MACAROON config")
if (!process.env.LND2_LOOP_MACAROON)
throw new ConfigError("Missing LND2_LOOP_MACAROON config")
if (getCronConfig().swapEnabled) {
Copy link
Collaborator Author

@openoms openoms Oct 27, 2022

Choose a reason for hiding this comment

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

this should be part of service validation

copied from #1733 (comment)

Copy link
Contributor

@ntheile ntheile Oct 27, 2022

Choose a reason for hiding this comment

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

i did not think we were allowed to call process.env from any other file other than process.ts? should i just set it to blank if it does not exist and then in the loopservice i can validate?

@@ -41,6 +43,9 @@ export const LoopService = ({
lndInstanceName,
}: LoopdConfig): ISwapService => {
const mac = Buffer.from(macaroon, "base64").toString("hex") as Macaroon
if (!tlsCert) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

probably here should be the validation, also service should not throw exceptions. Please check onchain service as example

copied from #1733 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

This should not ever happen - either config is set and tlsCert is never null or it is not set and this code should never be called see eab6cd4

@bodymindarts
Copy link
Member

I have added a commit eab6cd4 that throws in the config if we try to access the loop config in the event that enabledSwap = false... it should never be called so we can flag the error right there.

@ntheile
Copy link
Contributor

ntheile commented Oct 27, 2022

Yeah this commit looks good and makes sense.

I am still having trouble getting tests to run locally when testing this change, similar error to the e2e test TypeError: Cannot read properties of undefined (reading 'tracingServiceName')

@openoms
Copy link
Collaborator Author

openoms commented Oct 27, 2022

I am still having trouble getting tests to run locally when testing this change, similar error to the e2e test TypeError: Cannot read properties of undefined (reading 'tracingServiceName')

Not sure why the tracingConfig came in the picture here, but ce26a65 lets the tests pass.

@ntheile
Copy link
Contributor

ntheile commented Oct 27, 2022

i tested it and it works locally now. i would defer to @bodymindarts about the tracing config

@ntheile
Copy link
Contributor

ntheile commented Oct 27, 2022

I created a custom docker image of this commit and it works with signet on my local dev charts node! Here is the config I set:

dev/galoy/galoy-signet-values.yml

galoy:
  network: signet
  images:
    ## Custom Test Galoy Application Image details
    app:
      repository: ntheile/galoy-test:0.0.9
      digest: "sha256:622984b81ad44d730666f1b95379978bf24156c540ec39e60ca1f9361db5dc8e"
      git_ref: "622984b"

  config:
    test_accounts:
      - ref: A
        phone: "INSERT_PHONE"
        code: "INSERT_CODE"
        role: "bankowner"
        username: "bankowner"
        ref: "bankowner"
        needUsdWallet: true
    apollo:
      playground: true
    cronConfig:
      swapEnabled: false

     ....

@openoms openoms merged commit 11f878c into main Oct 28, 2022
@openoms openoms deleted the nolooponsignet branch October 28, 2022 07:10
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