feat(backend): unique keys per wallet address

[sabineschaller]

Changes proposed in this pull request

This prevents uploading the same key per wallet address multiple times.

Context

• follow up to [BUG] Revoking key didn't actually revoke it #2795

Checklist

• Related issues linked using fixes #number
• Tests added/updated
• Documentation added
• Make sure that all checks pass
• Bruno collection updated

[netlify]

✅ Deploy Preview for brilliant-pasca-3e80ec ready!

Name	Link
🔨 Latest commit	83664de
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/66bf1b479a946800085bcfa7
😎 Deploy Preview	https://deploy-preview-2863--brilliant-pasca-3e80ec.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[BlairCurrey]

played around with it and it seems to be working. Had to lookup ctid but I guess it probably makes for an easier query than using the uuid.

What's the user experience going to be for the people using the keys being deleted here? I recall the discussion and read the issue but I guess I'm still not quite connecting the dots. For example, is some normal user going to see invalid signature or similar from the grant request process and have no idea they need to upload some new key? I guess probably not because they should still have a key right (just not duplicates of it)? I suppose maybe it just fixes the bug in that issue and otherwise people wont be impacted from these duplicates being deleted.

I see the error for duplicate writes in gql - that makes sense.

[sabineschaller]

The users should not have any issues since they still have another copy of the key. But this makes me think I need to check what happens when a user re-adds a key that they have previously revoked. That should be un-revoked and not throw the duplicate error. And I'll also double check that we actually delete the older versions of the key in the migration and keep the most recent.

[BlairCurrey]

yeah thats a good point... since we're not deleting these and instead setting revoked to true I imagine re-adding the previously revoked key will trigger the unique constraint error. I dont think we can just add the revoked to the unique constraint because you couldnt revoke it twice.

1. add key, OK
2. revoke key, OK
3. re-add same key, OK
4. revoke same key, NOT OK. already have a revoked key with these unique fields.

[BlairCurrey]

Not tested, but I think a partial index like this would work:

CREATE UNIQUE INDEX wallet_address_keys_revoked_false_idx
ON "walletAddressKeys" (walletAddressId, kid, x)
WHERE revoked = false;

[golobitch]

do we really want to unrevoke the key? if we do this, we lose the information that sometime in the past, this key was revoked. I think that we need to have new entry in the database if revoked key was again added

[mkurapov]

Agreed with @golobitch re: unrevoking (we should just create a new row instead) & @BlairCurrey's partial index suggestion

[BlairCurrey]

noticed this wasn't actually running the DELETE (and when it does postgres doesnt like the unquoted camelCase names). Need to do something like:

Suggested change

	exports.up = function (knex) {
	// delete any existing duplicates per wallet address
	knex.raw(`
	DELETE FROM walletAddressKeys
	WHERE ctid NOT IN (
	SELECT MIN(ctid)
	FROM walletAddressKeys
	GROUP BY walletAddressId, kid, x
	);
	`)
	return knex.schema.alterTable('walletAddressKeys', (table) => {
	table.unique(['walletAddressId', 'kid', 'x'])
	})
	}
	exports.up = function (knex) {
	// delete any existing duplicates per wallet address
	return knex
	.raw(
	`
	DELETE FROM "walletAddressKeys"
	WHERE ctid NOT IN (
	SELECT MIN(ctid)
	FROM "walletAddressKeys"
	GROUP BY "walletAddressId", kid, x
	);
	`
	)
	.then(() => {
	return knex.schema.alterTable('walletAddressKeys', (table) => {
	table.unique(['walletAddressId', 'kid', 'x'])
	})
	})
	}

Tested locally and it works.

Its kind of annoying but when I want to test migrating data I:

1. move the file out of the migration dir
2. spin up the containers and add whatever data that needs to be migrated (adding a duplicate key in this case)
3. stop the containers (dont remove the volume) and move migration file back in
4. spin back up and check

[golobitch]

Yes, in PostgreSQL you need to put camel case in quotes. Delete statement was not performed probably because of the missing await, because it is async operation.

[BlairCurrey]

wish we would have used the snake case mapper when we setup the db initially: https://vincit.github.io/objection.js/recipes/snake-case-to-camel-case-conversion.html

[sabineschaller]

I'll add the quotes.

@golobitch all of these have to be async functions?
EDIT: I googled, looks like just the up needs to be async.

[BlairCurrey]

With knex and objection, if you dont await the function the query is not executed. Instead it returns a query builder (so you can modify further).

That's what happened originally and asserting as much here really made me stop and think how that was even possible (aka, "am I wrong???"). Guess I simply was not familiar with then() (MDN).

• A SO post asking the same thing: https://stackoverflow.com/questions/65319822/how-does-knex-await-execute-a-database-query
• a little knex query builder prototype that connects all the dots https://gist.github.com/BlairCurrey/ece2557b75487fc2bec6c006526b0324

[BlairCurrey]

I've found there to be a lot of gotchas with promises in the knex migrations. Another important thing is returning from the migration (which we are doing). We couldn't, for example, await all the queries and not return (something I've done).

I've also seen/tried using Promise.All like this but I don't think it would guarantee resolution order because they run concurrently (ie, second might actually modify the db before the first):

return Promise.All([
  knex.raw(...),
  knex.raw(...)
])

Usually I actually use the dreaded promise chaining 😱 :

return knex.raw(...)
 .then(()=>{
   knex.raw(...)
 })

Ugly, I know. But clear.

The way it is now seems fine - just wanted to chime in with other general pitfalls I've run into.

[golobitch]

@sabineschaller No, I meant that knex.raw needs to be awaited

[BlairCurrey]

hmmm what's going on with the history? Just surprised to see a bunch of commits already in main.

[sabineschaller]

hmmm what's going on with the history? Just surprised to see a bunch of commits already in main.

@BlairCurrey I messed up merging in main. I haven't touched code in a while so I'm out of practice 😬

[mkurapov]

@sabineschaller to double check, are we good to review this?