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

Connect user account methods to PG #298

Merged
merged 14 commits into from
Mar 30, 2023
Merged

Connect user account methods to PG #298

merged 14 commits into from
Mar 30, 2023

Conversation

ccali11
Copy link
Contributor

@ccali11 ccali11 commented Mar 28, 2023

@shanejearley - Focused on connecting the following actions to PG and cleaning up code a bit (ongoing and iterative process):

  • signup/login (we can just called login)
  • add account
  • remove account

Of note:

  • I still need to work through the user action of updating primary address (easy to do in-memory; requires more careful thought with PG); for now I disabled that feature
  • Worth reviewing types/schemas with you and/or @hawyar to see how we can keep them in sync to the greatest degree possible (@hawyar and I already met on this once, but it's worth another touch base)
    • Also a specific outstanding design choice I'm still thinking through on the topic of types/schemas: can multiple users (owners of an address) be able to share an address/account? For example, can I have two primary wallets (which would correspond to two separate users) that both share some third address in common? If so, the current schema for account / the accounts table prohibits that since that third/shared address would be a primary key on the accounts table. Can discuss more, but otherwise just know I'm still thinking through this.
  • There are some outstanding TS issues as a result of the PascalCase formatting you've suggested; we should briefly settle on how we'd like to represent property names in the code (and if they should be different in different areas of the code)

@shanejearley
Copy link
Contributor

Thanks Chris! Can you elaborate on the PascalCase issue to jog my memory? Glad you spotted the shared third account challenge. If we want, I figured we could change owner to owners as a column on accounts and for users join accounts on owners inclusion rather than owner equality. Can discuss here or online. Let me know if you'd prefer a separate issue to work that out.

@hawyar
Copy link
Contributor

hawyar commented Mar 28, 2023

Once we finalize the JSON Schema for Account and User and create their interfaces we have to be careful not to mix additional application code types with the original interface.

For example, if the frontend decides to add a new field SnapshotIndex to Account for charting convenience. Instead of adding it to directly to the Account as an optional field we would create a new interface AccountWithSnapshotIndex that has both.

interface AccountWithSnapshotIndex {
    Account: Account
    SnapshotIndex?: []
}

This would be like treating the original interface as generated code (we will do that soon) which we are only allowed to derive new types from it not mutate it. Only a JSON Schema change would be allowed to update that original interface.

Great work @ccali11

@ccali11
Copy link
Contributor Author

ccali11 commented Mar 29, 2023

Thanks Chris! Can you elaborate on the PascalCase issue to jog my memory? Glad you spotted the shared third account challenge. If we want, I figured we could change owner to owners as a column on accounts and for users join accounts on owners inclusion rather than owner equality. Can discuss here or online. Let me know if you'd prefer a separate issue to work that out.

@shanejearley - You have a method that formats PG return values to PascalCase. You implemented it before you handed the PG and users API infrastructure over to me. So when I return data/user from backend to front-end to set the user ref on the frontend, the attributes on the data/user are PascalCase such as user.value.Address or user.value.Accounts. This causes type collisions because our types are formatted in camelCase so I'm getting TS issues. I could reformat back to camelCase on the front-end, or I could just format the values to camelCase instead of PascalCase on the backend. But wanted to understand if you made it PascalCase on backend for a reason before I changed anything - Chesterton's Fence, if you will.

@shanejearley - And yes, I would like it if we could merge this PR and we could tackle the multiple users with same account design in a separate PR. Could probably also tackle the changePrimaryAddress issue there as well.

@ccali11 ccali11 closed this Mar 29, 2023
@ccali11 ccali11 reopened this Mar 29, 2023
@ccali11
Copy link
Contributor Author

ccali11 commented Mar 29, 2023

Once we finalize the JSON Schema for Account and User and create their interfaces we have to be careful not to mix additional application code types with the original interface.

For example, if the frontend decides to add a new field SnapshotIndex to Account for charting convenience. Instead of adding it to directly to the Account as an optional field we would create a new interface AccountWithSnapshotIndex that has both.

interface AccountWithSnapshotIndex {
    Account: Account
    SnapshotIndex?: []
}

This would be like treating the original interface as generated code (we will do that soon) which we are only allowed to derive new types from it not mutate it. Only a JSON Schema change would be allowed to update that original interface.

Great work @ccali11

@hawyar - Could use a quick huddle to better understand the finality of schema concept. Are you saying we have to freeze the schema at some point? I can understand if so (e.g., it affects your crawler and we wouldn't want to be constantly making changes to one and not the other sort of thing...).

@shanejearley
Copy link
Contributor

Great catch Chris, my mistake. We wanted to use the camelCase helper here.

Nice that the parameterized queries don't require snakeCase on the other end.

I think he means schema change = base types and interfaces change by codegen. Then we build custom types wherever we need that utilize the base from codegen. Live changing the schema during development is also doable so nothing is frozen.

@shanejearley
Copy link
Contributor

@hawyar do you have a diagram to add to the README?

@shanejearley
Copy link
Contributor

@ccali11 ready to merge.

@ccali11 ccali11 merged commit 270a47b into develop Mar 30, 2023
@ccali11 ccali11 deleted the feature/pg-users branch March 30, 2023 16:11
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