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

Add GetInboxIDs and use validation service in PublishIdentityUpdates #382

Merged
merged 37 commits into from
Apr 25, 2024

Conversation

neekolas
Copy link
Collaborator

@neekolas neekolas commented Apr 25, 2024

tl;dr

  • Adds methods for getting the Address Log
  • Adds updates to address log in the PublishIdentityUpdates flow for new and revoked members

Copy link
Collaborator Author

neekolas commented Apr 25, 2024

@neekolas neekolas changed the title Add GetAddressLog SQL WIP Add GetAddressLog SQL Apr 25, 2024
@insipx insipx mentioned this pull request Apr 25, 2024
…m:xmtp/xmtp-node-go into 04-24-migrate_first_store_methods_to_sqlc
@insipx insipx mentioned this pull request Apr 25, 2024
…m:xmtp/xmtp-node-go into 04-24-migrate_first_store_methods_to_sqlc
@neekolas neekolas force-pushed the 04-24-migrate_first_store_methods_to_sqlc branch from b6a8db2 to 4e985e1 Compare April 25, 2024 19:28
Base automatically changed from 04-24-migrate_first_store_methods_to_sqlc to main April 25, 2024 20:11
neekolas and others added 15 commits April 25, 2024 13:26
* Add insertlog query

* validation service

* insert log

* revocation for removed members

* lint

* remove unnecessary log

* change test to use query from sqlc

* remove comments

* fix tests
Bun has been bothering me for a while, so I've been working on a migration that will get rid of our ORM altogether and just use boring SQL queries for everything.

[`sqlc`](https://sqlc.dev/) is a very slick tool to generate code based on plain SQL queries using placeholders for arguments. It's not perfect...I had to do some gymnastics to make a few of the query types work.

But the fact that there is no runtime other than the standard SQL driver and some generated code outweighs its limitations IMO. There's no fancy ORM library to worry about mangling your queries, and the learning curve is basically just "how well do you know SQL".

- No support for serializable transactions
- The SQL driver is not as well maintained as PGX
- High learning curve to build complex queries, even if you know SQL well
- Relations system is not very powerful and ends up doing N+1 queries a lot of the time.
- Configuring the database with struct tags is errorprone, and there aren't great checks to make sure the struct tags actually match the schema.

- I can't find a good way to have dynamic ORDER BY expressions. So I literally have separate queries for ASC and DESC versions. It's not the end of the world, but it's very frustrating. There's an [issue to fix it](sqlc-dev/sqlc#2061), and some hacky workarounds using CASE statements, but it's not great.
- Making the filters play nice with `json_populate_recordset` is a bit of a pain. Switching to the `pgx` driver helped, since I think there was a bug in Bun's pgdriver.

We use Bun in a lot of places and for a lot of things today.

- It powers the `authz` database and all the migrations there
- It powers the migrations for the `message` database (but not the queries)
- It powers the `mls` database and all the queries in the `mlsstore`.

The priority right now is to remove it from the `mlsstore`. We will still use it for migrations (`sqlc` can read Bun migrations just fine).

This involves replacing the bun `pgdriver` with `pgx` (done in this PR) and replacing all the Bun ORM queries with `sqlc` queries. I have most of the queries written, but I'll split up the actual migration over several PRs. This can be done incrementally, but once the process is complete we can delete the Bun models.

We aren't using any of the fancy `sqlc` cloud features and have no plans to.

Ummmm. 😬. That was me.
* Add insertlog query

* validation service

* insert log

* revocation for removed members

* lint

* remove unnecessary log

* change test to use query from sqlc

* remove comments

* fix tests
…m:xmtp/xmtp-node-go into 04-24-migrate_first_store_methods_to_sqlc
* Add insertlog query

* validation service

* insert log

* revocation for removed members

* lint

* remove unnecessary log

* change test to use query from sqlc

* remove comments

* fix tests
* Add insertlog query

* validation service

* insert log

* revocation for removed members

* lint

* remove unnecessary log

* change test to use query from sqlc

* remove comments

* fix tests
@neekolas neekolas requested a review from insipx April 25, 2024 21:27
@neekolas neekolas marked this pull request as ready for review April 25, 2024 21:27
@neekolas neekolas changed the title WIP Add GetAddressLog SQL Add GetInboxIDs and use validation service in PublishIdentityUpdates Apr 25, 2024
@neekolas neekolas enabled auto-merge (squash) April 25, 2024 21:32
@neekolas neekolas merged commit cb1822f into main Apr 25, 2024
4 checks passed
@neekolas neekolas deleted the 04-25-add_getaddresslog_sql branch April 25, 2024 21:36
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.

2 participants