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 canceled.

Name	Link
🔨 Latest commit	424cce6
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/67614daf1dfaf10009792715

[mkurapov]

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

[oana-lolea]

Changes:

• Added unrevoke wallet address key query resolver
• Updated migration script to create a unique index on walletAddressId, kid and x, when the key is not revoked, because we can have the same key revoked multiple times (useful for keeping the key's history)
• Added tests for the service
• Added bruno file for unrevoking

[oana-lolea]

Updates based on #3130.

1. Updated migration to:

• delete all the duplicate active keys of a wallet address, keeping the most recent one.
• unique constraint is now on (walletAddressId, kid), to avoid having duplicate keys.

2. Removed unrevoke resolver, because create does exactly what unrevoke is supposed to do: create a new row with a key that is not revoked.

3. Tested that UniqueConstraint error is returned when trying to add the same key that is not revoked.

[mkurapov]

Looks like you are already handling this in the "'cannot add duplicate key to a wallet address'" test.

[mkurapov]

I think we can just add this to the "create" describe block

[mkurapov]

This is a tricky one. One thing we need to add is the kid parameter (so we don't delete all non-revoked keys for a wallet address), and also maybe we can come up with a way to not delete by createdAt (although very unlikely, it could end up deleting valid, non-duplicated keys which happen to have the same createdAt).

[oana-lolea]

Yeah, I think that grouping by kid is the right way to do it.

Suggested change

	`DELETE FROM "walletAddressKeys" w
	WHERE revoked = false
	AND "createdAt" <> (
	SELECT MAX("createdAt")
	FROM "walletAddressKeys"
	WHERE revoked = false
	AND "walletAddressId" = w."walletAddressId"
	GROUP BY "walletAddressId"
	);
	`DELETE FROM "walletAddressKeys" w
	WHERE revoked = false
	AND "createdAt" <> (
	SELECT MAX("createdAt")
	FROM "walletAddressKeys"
	WHERE revoked = false
	AND kid = w.kid
	GROUP BY kid
	);

Could we use TOP to delete just one key if there are multiple valid keys with the same createdAt? I don't see another way to determine which key to choose.

[oana-lolea]

This is a way to do it, but still based on createdAt. If there are multiple keys that have the most recent createdAt, we can keep the first one we find and delete the rest. What do you think, @mkurapov ?

Suggested change

	`DELETE FROM "walletAddressKeys" w
	WHERE revoked = false
	AND "createdAt" <> (
	SELECT MAX("createdAt")
	FROM "walletAddressKeys"
	WHERE revoked = false
	AND "walletAddressId" = w."walletAddressId"
	GROUP BY "walletAddressId"
	);
	`DELETE FROM "walletAddressKeys" w
	WHERE revoked = false
	AND w."createdAt" = (
	SELECT "createdAt"
	FROM "walletAddressKeys"
	WHERE revoked = false
	AND kid = w.kid
	ORDER BY "createdAt" DESC
	LIMIT 1
	)
	AND w.id <> (
	SELECT id
	FROM "walletAddressKeys"
	WHERE revoked = false
	AND kid = w.kid
	ORDER BY "createdAt" DESC
	LIMIT 1 OFFSET 1
	);
	`

[mkurapov]

@oana-lolea I was looking through the PR history, and I think we can use physical location row ctid value:

rafiki/packages/backend/migrations/20240731084359_add_unique_constraint_to_kid.js

Lines 8 to 13 in 5e81569

	DELETE FROM walletAddressKeys
	WHERE ctid NOT IN (
	SELECT MIN(ctid)
	FROM walletAddressKeys
	GROUP BY walletAddressId, kid, x
	);

I agree with you on keeping the most recent key so we'd have to adjust the migration a bit, but using ctid should work.

[mkurapov]

I think we need to group by walletAddressId and kid here still, otherwise we may delete keys that have the same kid but belong to a different wallet address.

Additionally, we should also use MAX(ctid) so we can grab the most recent key

[mkurapov]

Suggested change

	`DELETE FROM "walletAddressKeys" w
	WHERE revoked = false
	AND ctid NOT IN (
	SELECT MAX(ctid)
	FROM "walletAddressKeys"
	WHERE revoked = false
	AND kid = w.kid
	GROUP BY kid, "walletAddressId"
	)`
	`DELETE FROM "walletAddressKeys" w
	AND ctid NOT IN (
	SELECT MAX(ctid)
	FROM "walletAddressKeys"
	WHERE revoked = false
	GROUP BY kid, "walletAddressId"
	)`

can you double check if this suggestion has the same behaviour? I think it should. Otherwise, looks good 👍