[ANCHOR-342] Add `clients` to the configuration #946

lijamie98 · 2023-06-15T20:59:34Z

PR Checklist

PR Structure

This PR has reasonably narrow scope (if not, break it down into smaller PRs).
This PR avoids mixing refactoring changes with feature changes (split into two PRs
otherwise).
This PR's title starts with name of package that is most changed in the PR, ex.
paymentservice.stellar, or all or doc if the changes are broad or impact many
packages.

Thoroughness

This PR adds tests for the most critical parts of the new functionality or fixes.

What

Add client_attribution to the configuration.

Why

This is to support the client status callback feature.

stellar-jenkins · 2023-06-15T21:02:55Z

Reference Server Preview is available here:
https://anchor-ref-pr946.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr946.previews.kube001.services.stellar-ops.com/

stellar-jenkins · 2023-06-15T21:04:32Z

Something went wrong with PR preview build please check

platform/src/main/resources/config/anchor-config-default-values.yaml

stellar-jenkins · 2023-06-15T21:08:07Z

Reference Server Preview is available here:
https://anchor-ref-pr946.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr946.previews.kube001.services.stellar-ops.com/

platform/src/main/resources/config/anchor-config-default-values.yaml

Ifropc · 2023-06-15T21:12:13Z

Can we deprecate

  ## @param: omnibus_account_list
  ## A comma-separated list of omnibus accounts (comma separated).
  ## The SEP-10 omnibus account is described at:
  ##     https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0010.md#memos
  #
  omnibus_account_list:
  ## @param: require_known_omnibus_account
  ## @type: bool
  ## Whether to require authenticating clients to be in the omnibusAccountList
  #
  require_known_omnibus_account: false

Because with new schema we can always just use client_attribution

stellar-jenkins · 2023-06-15T21:12:31Z

Reference Server Preview is available here:
https://anchor-ref-pr946.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr946.previews.kube001.services.stellar-ops.com/

Ifropc · 2023-06-15T21:12:52Z

platform/src/main/resources/config/anchor-config-default-values.yaml

+# Examples:
+#     - domain: lobstr.com
+#       type: non-custodial
+#       public_key: GBI2IWJGR4UQPBIKPP6WG76X5PHSD2QTEBGIP6AZ3ZXWV46ZUSGNEGN2


Does non-custodial wallet even have a public key? I think it should only be required for custodial wallets

Doesn't lobstr.com server has a SIGNING_KEY in the TOML file?

cc: @JakeUrban

Noncustodial wallets do have a public key, published on their stellar.toml file. This is nice because the non-custodial wallet could rotate their signing key and the anchor's authentication process would still work because it should fetch the SIGNING_KEY on each authentication request.

For this same reason, I don't think we should require each wallet entry to have a public key attribute. Only custodial wallets will need the public_key attribute.

I think we can add type when there is an immediate use case. In that case, the public_key will stay.

lijamie98 · 2023-06-16T16:25:13Z

Can we deprecate

  ## @param: omnibus_account_list
  ## A comma-separated list of omnibus accounts (comma separated).
  ## The SEP-10 omnibus account is described at:
  ##     https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0010.md#memos
  #
  omnibus_account_list:
  ## @param: require_known_omnibus_account
  ## @type: bool
  ## Whether to require authenticating clients to be in the omnibusAccountList
  #
  require_known_omnibus_account: false

Because with new schema we can always just use client_attribution

Is there a use case where a wallet uses omnibus_account without client_attribution?

JakeUrban

Made some comments / suggestions. Regarding backwards compatibility, I think we should deprecate the allowlist & denylist attributes but keep the client_attribution_required flag. Then we'll validate that everything matches on startup if the wallets object is present.

platform/src/main/resources/config/anchor-config-default-values.yaml

JakeUrban · 2023-06-16T21:02:51Z

platform/src/main/resources/config/anchor-config-default-values.yaml

+# Examples:
+#     - domain: lobstr.com
+#       type: non-custodial
+#       public_key: GBI2IWJGR4UQPBIKPP6WG76X5PHSD2QTEBGIP6AZ3ZXWV46ZUSGNEGN2


Noncustodial wallets do have a public key, published on their stellar.toml file. This is nice because the non-custodial wallet could rotate their signing key and the anchor's authentication process would still work because it should fetch the SIGNING_KEY on each authentication request.

For this same reason, I don't think we should require each wallet entry to have a public key attribute. Only custodial wallets will need the public_key attribute.

JakeUrban · 2023-06-16T21:12:39Z

platform/src/main/resources/config/anchor-config-default-values.yaml

+  # The flag to enable the status callback to the client domain
+  client_domain_status_callback_enabled: true
+  # The flag to enable the event delivery to the anchor business server
+  callback_api_event_delivery_enabled: true


I don't love the naming here. I think something generalized would be better:

events: transaction_status_updated: wallet_callbacks_enabled: true callback_api_requests_enabled: true

This way, we can add more event types and more handlers of those events using a predefined structure.

There is a event section that defines the existing EventService. This is specific to event-processor.

I would probably swap the structure like. Please refer to the updated commit.

I see, I don't mind using the event-processor top-level object.

What do you think about using event type as a sub-object though? That way the business could subscribe to a subset of events, instead of having just a single true/false attribute for all event types.

For example,

event_processor: event_type: transaction_status_updated: client_callback_enabled: true callback_api_requests_enabled: true transaction_created: client_callback_enabled: false callback_api_requests_enabled: true

I don't feel strongly about this. If we want to use a single attribute for all requests that it ok, we'll just have less flexibility and need to make assumptions about what anchors and clients will want.

Not all callbacks are interested in all event types. How about we do this:

event_processor: # The context path of the event processing server context_path: / # The listening port of the event processing server port: 8084 # The configuration of the status callback to the client domain client_domain_status_callback: enabled: true # The configuration of the event delivery to the anchor business server callback_api_event_delivery: enabled: true

This will leave more room for customization and we don't have to define the behavior of the events unless it is necessary to customize.

Hey Jamie, can we use the following names, client_status_callback and callback_api_requests?

Can we use singular callback_api_request because client_status_callback is singular.

Works for me!

platform/src/main/resources/config/anchor-config-default-values.yaml

stellar-jenkins · 2023-06-21T18:24:42Z

Reference Server Preview is available here:
https://anchor-ref-pr946.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr946.previews.kube001.services.stellar-ops.com/

stellar-jenkins · 2023-06-21T18:36:28Z

Reference Server Preview is available here:
https://anchor-ref-pr946.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr946.previews.kube001.services.stellar-ops.com/

stellar-jenkins · 2023-06-21T20:57:02Z

Reference Server Preview is available here:
https://anchor-ref-pr946.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr946.previews.kube001.services.stellar-ops.com/

lijamie98 · 2023-06-27T15:41:45Z

I am not sure if we may deprecate the omnibus_account_list.

omnibus_account_list seems to be inconsistent with client.
I think it is possible omnibus_account_list can be different from the wallet_addresss defined in the clients section.

stellar-jenkins · 2023-06-27T15:52:56Z

Reference Server Preview is available here:
https://anchor-ref-pr946.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr946.previews.kube001.services.stellar-ops.com/

JakeUrban · 2023-06-27T19:12:53Z

I am not sure if we may deprecate the omnibus_account_list.
omnibus_account_list seems to be inconsistent with client.
I think it is possible omnibus_account_list can be different from the wallet_addresss defined in the clients section.

Can you provide an example of how it is inconsistent? I think the omnibus account list is directly associated with the auth_accounts of a given client.

stellar-jenkins · 2023-06-27T21:49:58Z

Reference Server Preview is available here:
https://anchor-ref-pr946.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr946.previews.kube001.services.stellar-ops.com/

stellar-jenkins · 2023-06-27T22:07:20Z

Reference Server Preview is available here:
https://anchor-ref-pr946.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr946.previews.kube001.services.stellar-ops.com/

lijamie98 requested review from JakeUrban and Ifropc June 15, 2023 21:04

Ifropc reviewed Jun 15, 2023

View reviewed changes

platform/src/main/resources/config/anchor-config-default-values.yaml Outdated Show resolved Hide resolved

lijamie98 force-pushed the feature/anchor-342-client-attribution-configuration branch from 0cb9670 to 7af8a1c Compare June 15, 2023 21:05

Ifropc reviewed Jun 15, 2023

View reviewed changes

platform/src/main/resources/config/anchor-config-default-values.yaml Show resolved Hide resolved

Ifropc reviewed Jun 15, 2023

View reviewed changes

JakeUrban reviewed Jun 16, 2023

View reviewed changes

lijamie98 force-pushed the feature/anchor-342-client-attribution-configuration branch from 8be50e0 to 41af11e Compare June 21, 2023 18:22

lijamie98 force-pushed the feature/anchor-342-client-attribution-configuration branch from ca6f08f to ffd14b9 Compare June 27, 2023 15:49

lijamie98 changed the title ~~[ANCHOR-342] Add client_attribution to the configuration~~ [ANCHOR-342] Add clients to the configuration Jun 27, 2023

Ifropc approved these changes Jun 27, 2023

View reviewed changes

JakeUrban approved these changes Jun 27, 2023

View reviewed changes

lijamie98 added 6 commits June 27, 2023 14:46

Add client_attribution to the configuration

3919b83

Fix typos

5c67005

Revised for backward compatibility

f8244f5

revised for feedback

f3c5d5d

Refactored event_processor configuration.

9cf8df4

Take care of comments

474714b

renamed fields

f76a8a5

lijamie98 force-pushed the feature/anchor-342-client-attribution-configuration branch from ffd14b9 to f76a8a5 Compare June 27, 2023 21:47

fix schema

c02dabf

lijamie98 merged commit 5349dbf into stellar:develop Jun 28, 2023
5 checks passed

lijamie98 self-assigned this Jul 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ANCHOR-342] Add `clients` to the configuration #946

[ANCHOR-342] Add `clients` to the configuration #946

lijamie98 commented Jun 15, 2023 •

edited

Loading

stellar-jenkins commented Jun 15, 2023

stellar-jenkins commented Jun 15, 2023

stellar-jenkins commented Jun 15, 2023

Ifropc commented Jun 15, 2023

stellar-jenkins commented Jun 15, 2023

Ifropc Jun 15, 2023

lijamie98 Jun 16, 2023

JakeUrban Jun 16, 2023

lijamie98 Jun 21, 2023

lijamie98 commented Jun 16, 2023

JakeUrban left a comment

JakeUrban Jun 16, 2023

JakeUrban Jun 16, 2023

lijamie98 Jun 21, 2023

JakeUrban Jun 21, 2023

JakeUrban Jun 21, 2023

lijamie98 Jun 27, 2023

JakeUrban Jun 27, 2023

lijamie98 Jun 27, 2023

JakeUrban Jun 27, 2023

stellar-jenkins commented Jun 21, 2023

stellar-jenkins commented Jun 21, 2023

stellar-jenkins commented Jun 21, 2023

lijamie98 commented Jun 27, 2023

stellar-jenkins commented Jun 27, 2023

JakeUrban commented Jun 27, 2023

stellar-jenkins commented Jun 27, 2023

stellar-jenkins commented Jun 27, 2023

[ANCHOR-342] Add clients to the configuration #946

[ANCHOR-342] Add clients to the configuration #946

Conversation

lijamie98 commented Jun 15, 2023 • edited Loading

PR Structure

Thoroughness

What

Why

stellar-jenkins commented Jun 15, 2023

stellar-jenkins commented Jun 15, 2023

stellar-jenkins commented Jun 15, 2023

Ifropc commented Jun 15, 2023

stellar-jenkins commented Jun 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lijamie98 commented Jun 16, 2023

JakeUrban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stellar-jenkins commented Jun 21, 2023

stellar-jenkins commented Jun 21, 2023

stellar-jenkins commented Jun 21, 2023

lijamie98 commented Jun 27, 2023

stellar-jenkins commented Jun 27, 2023

JakeUrban commented Jun 27, 2023

stellar-jenkins commented Jun 27, 2023

stellar-jenkins commented Jun 27, 2023

[ANCHOR-342] Add `clients` to the configuration #946

[ANCHOR-342] Add `clients` to the configuration #946

lijamie98 commented Jun 15, 2023 •

edited

Loading