Skip to content

Add connected wallets endpoint#16

Merged
rickyrombo merged 1 commit intomainfrom
mjp-connected-wallets
Apr 11, 2025
Merged

Add connected wallets endpoint#16
rickyrombo merged 1 commit intomainfrom
mjp-connected-wallets

Conversation

@rickyrombo
Copy link
Copy Markdown
Contributor

Thoughts:

  • Testing the query and the endpoint seems fairly redundant in this case, but maybe the interesting thing is to test that the json keys are the correct and that the status code is correct
  • Was tempted to switch over to array_agg and do the grouping in the query, but had to map the chain values anyways so seemed moot. In v2, we should just return the list. I don't think the output formatting by chain is very useful or relevant
  • Making fixtures is somewhat nice, but I worry a bit about it discouraging us from setting up DB constraints to protect our data integrity?
  • Not sure FullConnectedWallets made sense to make. Maybe better to just inline the mapping to the api? (There's no Full/non-full version of this)

Copy link
Copy Markdown
Member

@raymondjacobson raymondjacobson left a comment

Choose a reason for hiding this comment

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

lgtm - i think that we should have full and non full for everything. if the response shape is the exact same, that's ok

@rickyrombo rickyrombo merged commit 8ca53de into main Apr 11, 2025
2 checks passed
@stereosteve
Copy link
Copy Markdown
Contributor

stereosteve commented Apr 11, 2025

Making fixtures is somewhat nice, but I worry a bit about it discouraging us from setting up DB constraints to protect our data integrity?

I think we can set up meaningful constraints and still use fixtures...
like the blocks constraint is annoying and also worthless.
Adding user_id constraints would make sense and also the fixtures should satisfy those constraints anyway.

@stereosteve stereosteve deleted the mjp-connected-wallets branch April 14, 2025 14:49
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