exp/services/recoverysigner: remove type, add identities #2323

leighmcculloch · 2020-02-26T01:35:47Z

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.
services/friendbot, 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.
I've updated any docs (developer docs, .md
files, etc... affected by this change). Take a look in the docs folder for a given service,
like this one.

Release planning

I've updated the relevant CHANGELOG (here for Horizon) if
needed with deprecations, added features, breaking changes, and DB schema changes.
I've decided if this PR requires a new major/minor version according to
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.

What

Remove the type parameter stored alongside registered accounts and replace it with exposing information about which identities are set on an account to clients.

Remove the type request and response parameters. Add the identities response parameter, and replace owner_identities and other_identities with identities.owner and identities.other.

Why

The type existed purely for clients to get some sense of what purpose an account was registered, but it's really meta data about how a client is using the service and isn't necessarily a core concept for the
service.

The value was also synonymous with which identities are configured for an account. A type of personal will always have only an owner identity, and a type of share was always going to have an other identity alone or in addition to owner.

By removing the concept of type and instead just giving a client visibility into which identities are set on the account we reduce the number of concepts a client has to learn and know about which makes the API simpler, even if the change appears to make the API slightly more complex.

The reason I replaced owner_identities and other_identities is while thinking about the right way to expose the identities to the client it occurred to me the simplest method for a JavaScript client would be to have the identities be keys of an object, and so I structured the response that way, and it made sense to also modify the request to match.

Known limitations

This is a breaking change to the API, but there are no consumers and the API is experimental.

### What Remove the `type` parameter stored alongside registered accounts and replace it with exposing information about which identities are set on an account to clients. ### Why The `type` existed purely for clients to get some sense of what purpose an account was registered, but it's really meta data about how a client is using the service and isn't necessarily a core concept for the service. The value was also synonymous with which identities are configured for an account. A `type` of `personal` will always have only an `owner` identity, and a `type` of `share` was always going to have an `other` identity alone or in addition to `owner`. By removing the concept of `type` and instead just giving a client visibility into which identities are set on the account we reduce the number of concepts a client has to learn and know about which makes the API simpler, even if the change appears to make the API slightly more complex.

leighmcculloch · 2020-02-26T01:38:22Z

exp/services/recoverysigner/internal/serve/account_list.go

+				Address: acc.Address,
+				Identities: accountResponseIdentities{
+					Owner: accountResponseIdentity{
+						Present: acc.OwnerIdentities.Present(),
+					},
+					Other: accountResponseIdentity{
+						Present: acc.OtherIdentities.Present(),
+					},
+				},


You'll notice this code block is duplicated a few times. I could reduce some code here, but I'm going to expend more effort holistically looking at reducing the duplicity here and in the authentication. Follow up: #2324.

howardtw · 2020-02-26T18:08:02Z

👀

howardtw · 2020-02-26T18:22:01Z

exp/services/recoverysigner/internal/serve/account_response.go

+}
+
+type accountResponseIdentities struct {
+	Owner accountResponseIdentity `json:"owner"`


I wonder if we can make these two fields omitempty instead of using the Present bool in another struct. If there's one field presented in the accountResponseIdentities, then the client should be able to tell whether it's owner or other.

I like that too, but our frontend team have mentioned before that it's much easier from a clients perspective is fields are always present and have a clear signal that something isn't available.

Got it. If that's the case, would making it IsOwner bool and IsOther bool be sufficient?

It is another way to solve the same goal, but it doesn't communicate the same intent. The account is not an owner, or an other, it has an owner identity, or an other identity.

If we ever want to expose more details about the owner or other the object let's us expand by adding more fields alongside present.

Do you have any specific concerns?

I usually avoid having nested objects that are too deep. However, if the goal is to incorporate more fields in the future, it makes sense. I still feel like the cleanest way is to make it the way I suggested in the first place as Present feels redundant. Let me confirm with the frontend team again.

I like the omitempty approach too. If I do that, what do we do when the thing is present but has no fields? It will be like this: "owner": {} which seems a bit odd and I think is harder to test for in JavaScript? Being able to do owner.present for true/false explicitly seems really simple.

It will be like this: "owner": {} which seems a bit odd and I think is harder to test for in JavaScript.

Maybe we can make the backend never do that?

Being able to do owner.present for true/false explicitly seems really simple.

Sure. It's not a blocker. Approving it.

exp/services/recoverysigner/internal/serve/account_response.go

marcelosalloum · 2020-02-26T18:52:53Z

exp/services/recoverysigner/internal/account/account.go

+
+// Present indicates if the Identities contains at least one identity. Returns
+// false if it is an empty/zero value.
+func (i Identities) Present() bool {


Nit: I personally like leading bool methods with Is, just to make things clearer:

Suggested change

func (i Identities) Present() bool {

func (i Identities) IsPresent() bool {

I've liked this pattern too in other languages in the past, but in Go code it's more common to omit the Is. Grabbing a random example the Response type has Close and Uncompressed, but there are loads of examples of this in the stdlib and other packages. Thankfully with types it is really clear that the returned value is a boolean. Let me know if this doesn't address your concern.

howardtw

Approving it. Note that there are still a couple of things that can potentially be improved like #2323 (comment), https://github.com/stellar/go/pull/2323/files#r384694003, and https://github.com/stellar/go/pull/2323/files#r384679399. We can address some of them in other PRs tho.

leighmcculloch requested review from marcelosalloum and howardtw February 26, 2020 01:35

leighmcculloch self-assigned this Feb 26, 2020

cla-bot bot added the cla: yes label Feb 26, 2020

leighmcculloch commented Feb 26, 2020

View reviewed changes

Merge branch 'master' into goodbye👋typesolongpeaceout✌️

afc619b

howardtw reviewed Feb 26, 2020

View reviewed changes

marcelosalloum approved these changes Feb 26, 2020

View reviewed changes

howardtw approved these changes Feb 26, 2020

View reviewed changes

leighmcculloch added 2 commits February 26, 2020 13:05

Remove type param no longer needed

822f8bc

Merge branch 'master' into goodbye👋typesolongpeaceout✌️

435260d

leighmcculloch merged commit c19fcdc into stellar:master Feb 26, 2020

leighmcculloch deleted the goodbye👋typesolongpeaceout✌️ branch February 26, 2020 21:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exp/services/recoverysigner: remove type, add identities #2323

exp/services/recoverysigner: remove type, add identities #2323

leighmcculloch commented Feb 26, 2020 •

edited

Loading

leighmcculloch Feb 26, 2020 •

edited

Loading

howardtw commented Feb 26, 2020

howardtw Feb 26, 2020

leighmcculloch Feb 26, 2020

howardtw Feb 26, 2020

leighmcculloch Feb 26, 2020 •

edited

Loading

howardtw Feb 26, 2020

leighmcculloch Feb 26, 2020

howardtw Feb 26, 2020

marcelosalloum Feb 26, 2020

leighmcculloch Feb 26, 2020 •

edited

Loading

howardtw left a comment

	func (i Identities) Present() bool {
	func (i Identities) IsPresent() bool {

exp/services/recoverysigner: remove type, add identities #2323

exp/services/recoverysigner: remove type, add identities #2323

Conversation

leighmcculloch commented Feb 26, 2020 • edited Loading

PR Structure

Thoroughness

Release planning

What

Why

Known limitations

leighmcculloch Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

howardtw commented Feb 26, 2020

howardtw Feb 26, 2020

Choose a reason for hiding this comment

leighmcculloch Feb 26, 2020

Choose a reason for hiding this comment

howardtw Feb 26, 2020

Choose a reason for hiding this comment

leighmcculloch Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

howardtw Feb 26, 2020

Choose a reason for hiding this comment

leighmcculloch Feb 26, 2020

Choose a reason for hiding this comment

howardtw Feb 26, 2020

Choose a reason for hiding this comment

marcelosalloum Feb 26, 2020

Choose a reason for hiding this comment

leighmcculloch Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

howardtw left a comment

Choose a reason for hiding this comment

leighmcculloch commented Feb 26, 2020 •

edited

Loading

leighmcculloch Feb 26, 2020 •

edited

Loading

leighmcculloch Feb 26, 2020 •

edited

Loading

leighmcculloch Feb 26, 2020 •

edited

Loading