[sql] added ethereum_address to users table
ClosedPublic
Actions

Authored by • derek on Nov 8 2022, 12:00 PM.

Details

Reviewers

varun
atul
ginsu
tomek
ashoat

Commits

rCOMM71c4b0f8b0b4: [sql] added ethereum_address to users table

Summary

self explanatory
linear task

Test Plan

run keyserver & watch it migrate.
also run

mysql -u root -p
...
DROP DATABASE comm;
CREATE DATABASE comm;

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• derek created this revision.Nov 8 2022, 12:00 PM

Herald added subscribers: • abosh, tomek. · View Herald TranscriptNov 8 2022, 12:00 PM

Harbormaster completed remote builds in B13289: Diff 18233.Nov 8 2022, 12:14 PM

• derek requested review of this revision.Nov 8 2022, 12:14 PM

I want to make sure @tomek takes a look since he has the most context on our MySQL setup

Overall looks good to me. Assume you've carefully tested that 42 chars is the right number?

In D5566#165047, @ashoat wrote:

Overall looks good to me. Assume you've carefully tested that 42 chars is the right number?

yeah, it'd be 40 but i figured we'd include 0x

Ah, I think we can skip 0x and save some space in the DB

In D5566#165096, @ashoat wrote:

Ah, I think we can skip 0x and save some space in the DB

I think that saving 2 bytes doesn't make any difference. What could make a difference is remembering in every place that we have to add 0x when reading from the db and removing it when writing. So for me we should keep it to make it more maintainable.

run keyserver & watch it migrate

It's also a good idea to check if creating an empty db also works correctly.

keyserver/src/database/migration-config.js
72 ↗	(On Diff #18233)	Shouldn't matter too much, but it's a good practice to make code idempotent if possible: so in this case `ADD COLUMN IF NOT EXISTS` would be slightly better.

This revision is now accepted and ready to land.Nov 9 2022, 5:53 AM

I see 0x as a purely visual treatment, so I would prefer to have that handled by the UI.

It's also a good idea to check if creating an empty db also works correctly.

@derek can you amend your Test Plan to cover that please?

keyserver/src/database/migration-config.js
72 ↗	(On Diff #18233)	Good call. This is something we probably should've learned from D5521

In D5566#165379, @ashoat wrote:

I see 0x as a purely visual treatment, so I would prefer to have that handled by the UI.

It's also a good idea to check if creating an empty db also works correctly.

@derek can you amend your Test Plan to cover that please?

all the frontend clients (wagmi, rainbowkit, even alchemy) give us the address with the 0x prefixed. i agree with tomek, it feels like a lot of roundtripping to add & remove it just for db writes to save a couple bytes

• derek edited the test plan for this revision. (Show Details)Nov 9 2022, 7:44 AM

updated migration to be idempotent

• derek requested review of this revision.Nov 9 2022, 7:45 AM

• derek marked 2 inline comments as done.

Harbormaster completed remote builds in B13322: Diff 18272.Nov 9 2022, 7:58 AM

Okay, the length of the field doesn't really matter. I won't block you on this, but it seems kinda wasteful to me

Please make sure you've tested all of this before landing

This revision is now accepted and ready to land.Nov 9 2022, 8:16 AM

Closed by commit rCOMM71c4b0f8b0b4: [sql] added ethereum_address to users table (authored by • derek). · Explain WhyNov 9 2022, 8:20 AM

This revision was automatically updated to reflect the committed changes.

• derek added a commit: rCOMM71c4b0f8b0b4: [sql] added ethereum_address to users table.