Skip to content
This repository was archived by the owner on May 14, 2021. It is now read-only.

Add one wallet endpoints#42

Merged
fuzzyami merged 14 commits intomasterfrom
add-one-wallet-endpoints
Jun 16, 2019
Merged

Add one wallet endpoints#42
fuzzyami merged 14 commits intomasterfrom
add-one-wallet-endpoints

Conversation

@fuzzyami
Copy link
Copy Markdown

No description provided.

Comment thread services/horizon/internal/init_web.go Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

  1. this isn't python world and no one is using underscore naming
  2. if you can, use a single action word instead of two or more
  3. search for RESTful API endpoint naming convention and pick something that makes sense

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The current horizon version uses underscore in an endpoint. if they can, surely we can too. refer to https://github.com/stellar/go/blob/009e6e45c9f514e8e4f102461fbaefdf92b4f448/services/horizon/internal/web.go#L179

Comment thread protocols/horizon/main.go Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

there's also account_id used in Horizon responses. Check the difference and keep it consistent with the rest of the responses Horizon does.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

from what I gather, "id" is used to identify the page returned. in some cases, its the account id but in others, it might a tx hash or another identifier for the page. In other words, I'll need to use account_id instead of id.

Comment thread protocols/horizon/main.go Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

possible account_id needed?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

if Action is a struct, use a pointer. if it's an interface it's ok

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

its done exactly this way in all the other actions.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

break lines here like in the above functions

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

when working with xdr. objects, do you need to work with copies or references?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

AFAIK, copies.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Test scenarios that we should test:

  1. master account which doesn't exist - test both endpoints
  2. master account which isn't linked to any other account - "
  3. matser account which is linked to another account - "
  4. matser account which is linked to two other accounts - "
  5. matser account which is linked to three other accounts - "
  6. test mass linking: you can use table test with the last three tests are, and test for linking 100+ accounts to a master account

edge cases:

  1. account A linked to B, which also links back to A - test all endpoints
  2. account A linked to B, account B linked to C - " + verify that A only shows B
  3. cyclical: account A linked to B, B linked to C, C linked to A - " + verify A only shows B
  4. repeat 1-3 but have A link to 3 normal accounts, and B as well to other 3 normal accounts. this is a "merge" of all test scenarios mention above this line.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should you use a pointer in the type here? verify with other implementations that already exist in the codebase

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

standard in go is to put comments on top of code, not side by side

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i would let @doodyparizada look at the queries as well

@fuzzyami fuzzyami force-pushed the add-one-wallet-endpoints branch from ad7bf31 to d07df03 Compare May 15, 2019 11:50
@fuzzyami fuzzyami force-pushed the add-one-wallet-endpoints branch from 0a9d8e6 to 2ea662c Compare June 13, 2019 13:58
@fuzzyami fuzzyami force-pushed the add-one-wallet-endpoints branch from 45a2e7b to 872fecd Compare June 16, 2019 06:18
@fuzzyami fuzzyami merged commit e4620e3 into master Jun 16, 2019
@fuzzyami fuzzyami deleted the add-one-wallet-endpoints branch June 16, 2019 06:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants