Skip to content

Commit 7586d7e

Browse files
lukabuzlukabuz
authored
fix: fix collision handling in comlink username task (#3293)
1 parent e1dd02c commit 7586d7e

File tree

3 files changed

+187
-46
lines changed

3 files changed

+187
-46
lines changed

‎indexer/packages/postgres/src/stores/subaccount-usernames-table.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,25 @@ export async function create(
8585
}).returning('*');
8686
}
8787

88+
export async function insertAndReturnCount(
89+
username: string,
90+
subaccountId: string,
91+
options: Options = { txId: undefined },
92+
): Promise<number> {
93+
const queryString: string = `
94+
INSERT INTO subaccount_usernames (username, "subaccountId")
95+
VALUES (?, ?)
96+
ON CONFLICT (username) DO NOTHING
97+
RETURNING username, "subaccountId";
98+
`;
99+
100+
const result: {
101+
rows: { username: string, subaccountId: string }[],
102+
} = await rawQuery(queryString, { ...options, bindings: [username, subaccountId] });
103+
104+
return result.rows.length;
105+
}
106+
88107
export async function findByUsername(
89108
username: string,
90109
options: Options = DEFAULT_POSTGRES_OPTIONS,

‎indexer/services/roundtable/__tests__/tasks/subaccount-username-generator.test.ts‎

Lines changed: 143 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,8 @@ import {
22
SubaccountUsernamesTable,
33
SubaccountTable,
44
QueryableField,
5-
65
testMocks,
76
dbHelpers,
8-
SubaccountUsernamesFromDatabase,
97
SubaccountFromDatabase,
108
testConstants,
119
} from '@dydxprotocol-indexer/postgres';
@@ -21,8 +19,12 @@ describe('subaccount-username-generator', () => {
2119
await testMocks.seedData();
2220
await testMocks.seedAdditionalSubaccounts();
2321
// delete all usernames that were seeded
24-
await SubaccountUsernamesTable.deleteBySubaccountId(testConstants.defaultSubaccountId);
25-
await SubaccountUsernamesTable.deleteBySubaccountId(testConstants.defaultSubaccountId2);
22+
await SubaccountUsernamesTable.deleteBySubaccountId(
23+
testConstants.defaultSubaccountId,
24+
);
25+
await SubaccountUsernamesTable.deleteBySubaccountId(
26+
testConstants.defaultSubaccountId2,
27+
);
2628
});
2729

2830
afterAll(async () => {
@@ -36,30 +38,154 @@ describe('subaccount-username-generator', () => {
3638
});
3739

3840
it('Successfully creates a username for all subaccount', async () => {
39-
const subaccounts: SubaccountFromDatabase[] = await
40-
SubaccountTable.findAll({
41-
subaccountNumber: 0,
42-
}, [QueryableField.SUBACCOUNT_NUMBER], {});
41+
const subaccounts: SubaccountFromDatabase[] = await SubaccountTable.findAll(
42+
{
43+
subaccountNumber: 0,
44+
},
45+
[QueryableField.SUBACCOUNT_NUMBER],
46+
{},
47+
);
4348

4449
const subaccountsLength: number = subaccounts.length;
45-
const subaccountsWithUsernames: SubaccountUsernamesFromDatabase[] = await
46-
SubaccountUsernamesTable.findAll(
47-
{}, [], {});
48-
expect(subaccountsWithUsernames.length).toEqual(0);
50+
const before = await SubaccountUsernamesTable.findAll(
51+
{},
52+
[],
53+
{ readReplica: true },
54+
);
55+
expect(before.length).toEqual(0);
4956

5057
await subaccountUsernameGenerator();
51-
const subaccountsWithUsernamesAfter: SubaccountUsernamesFromDatabase[] = await
52-
SubaccountUsernamesTable.findAll(
53-
{}, [], {});
58+
const after = await SubaccountUsernamesTable.findAll(
59+
{},
60+
[],
61+
{ readReplica: true },
62+
);
5463

5564
const expectedUsernames = [
5665
'BubblyEarH5Y', // dydx1n88uc38xhjgxzw9nwre4ep2c8ga4fjxc575lnf
5766
'GreenSnowWTT', // dydx1n88uc38xhjgxzw9nwre4ep2c8ga4fjxc565lnf
5867
'LunarMatFK5', // dydx199tqg4wdlnu4qjlxchpd7seg454937hjrknju4
5968
];
60-
expect(subaccountsWithUsernamesAfter.length).toEqual(subaccountsLength);
69+
expect(after.length).toEqual(subaccountsLength);
6170
for (let i = 0; i < expectedUsernames.length; i++) {
62-
expect(subaccountsWithUsernamesAfter[i].username).toEqual(expectedUsernames[i]);
71+
expect(after[i].username).toEqual(expectedUsernames[i]);
72+
}
73+
});
74+
75+
it('Falls back to a second username when there is a conflict on the first attempt', async () => {
76+
const subaccounts: SubaccountFromDatabase[] = await SubaccountTable.findAll(
77+
{
78+
subaccountNumber: 0,
79+
},
80+
[QueryableField.SUBACCOUNT_NUMBER],
81+
{},
82+
);
83+
const targetSubaccount = subaccounts[0];
84+
const otherSubaccount = subaccounts[1];
85+
86+
const { generateUsernameForSubaccount } = require('../../src/helpers/usernames-helper');
87+
88+
const usernameAttempt0 = generateUsernameForSubaccount(
89+
targetSubaccount.address,
90+
0,
91+
0,
92+
);
93+
await SubaccountUsernamesTable.create({
94+
username: usernameAttempt0,
95+
subaccountId: otherSubaccount.id,
96+
});
97+
98+
const afterPreInsert = await SubaccountUsernamesTable.findAll(
99+
{ subaccountId: [targetSubaccount.id] }, [QueryableField.SUBACCOUNT_ID], {},
100+
);
101+
expect(afterPreInsert.length).toBe(0);
102+
103+
await subaccountUsernameGenerator();
104+
105+
const created = await SubaccountUsernamesTable.findAll(
106+
{ subaccountId: [targetSubaccount.id] }, [QueryableField.SUBACCOUNT_ID], {},
107+
);
108+
expect(created.length).toBe(1);
109+
110+
const fallbackUsername = generateUsernameForSubaccount(
111+
targetSubaccount.address,
112+
0,
113+
1,
114+
);
115+
expect(created[0].username).toEqual(fallbackUsername);
116+
117+
const conflict = await SubaccountUsernamesTable.findAll(
118+
{ username: [usernameAttempt0] }, [QueryableField.USERNAME], {},
119+
);
120+
expect(conflict.length).toBe(1);
121+
expect(conflict[0].subaccountId).not.toEqual(targetSubaccount.id);
122+
});
123+
124+
it('Handles batch where one username succeeds and the other needs a fallback', async () => {
125+
const subaccounts: SubaccountFromDatabase[] = await SubaccountTable.findAll(
126+
{
127+
subaccountNumber: 0,
128+
},
129+
[QueryableField.SUBACCOUNT_NUMBER],
130+
{},
131+
);
132+
expect(subaccounts.length).toBeGreaterThanOrEqual(2);
133+
134+
const sub0 = subaccounts[0];
135+
const sub1 = subaccounts[1];
136+
137+
const { generateUsernameForSubaccount } = require('../../src/helpers/usernames-helper');
138+
139+
const sub0Attempt0 = generateUsernameForSubaccount(sub0.address, 0, 0);
140+
await SubaccountUsernamesTable.create({
141+
username: sub0Attempt0,
142+
subaccountId: sub1.id,
143+
});
144+
145+
// pre-run checks
146+
const preUsernames = await SubaccountUsernamesTable.findAll(
147+
{},
148+
[],
149+
{ readReplica: true },
150+
);
151+
expect(
152+
preUsernames.find((u: any) => u.username === sub0Attempt0),
153+
).toBeDefined();
154+
expect(preUsernames.filter((u: any) => u.subaccountId === sub0.id).length).toBe(0);
155+
expect(preUsernames.filter(
156+
(u: any) => u.subaccountId === sub1.id && u.username !== sub0Attempt0,
157+
).length).toBe(0);
158+
159+
// run generator
160+
await subaccountUsernameGenerator();
161+
162+
// fetch results
163+
const after = await SubaccountUsernamesTable.findAll(
164+
{},
165+
[],
166+
{ readReplica: true },
167+
);
168+
169+
// sub0 should have fallback username
170+
const sub0UsernameRow = after.find((u: any) => u.subaccountId === sub0.id);
171+
const sub0ExpectedFallback = generateUsernameForSubaccount(sub0.address, 0, 1);
172+
expect(sub0UsernameRow).toBeDefined();
173+
if (sub0UsernameRow) {
174+
expect(sub0UsernameRow.username).toEqual(sub0ExpectedFallback);
175+
}
176+
const sub1UsernameRow = after.find(
177+
(u: any) => u.subaccountId === sub1.id && u.username !== sub0Attempt0);
178+
if (sub1UsernameRow) {
179+
expect(sub1UsernameRow).toBeDefined();
180+
}
181+
182+
// There should not be two usernames with the same value
183+
const usernameCounts = after.reduce((acc: Record<string, number>, u: any) => {
184+
acc[u.username] = (acc[u.username] || 0) + 1;
185+
return acc;
186+
}, {});
187+
for (const count of Object.values(usernameCounts)) {
188+
expect(count).toBe(1);
63189
}
64190
});
65191
});

‎indexer/services/roundtable/src/tasks/subaccount-username-generator.ts‎

Lines changed: 25 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import { logger, stats } from '@dydxprotocol-indexer/base';
22
import {
33
SubaccountUsernamesTable,
4-
SubaccountsWithoutUsernamesResult,
54
Transaction,
65
} from '@dydxprotocol-indexer/postgres';
76
import _ from 'lodash';
@@ -12,9 +11,7 @@ import { generateUsernameForSubaccount } from '../helpers/usernames-helper';
1211
export default async function runTask(): Promise<void> {
1312
const taskStart: number = Date.now();
1413

15-
const subaccountZerosWithoutUsername:
16-
SubaccountsWithoutUsernamesResult[] = await
17-
SubaccountUsernamesTable.getSubaccountZerosWithoutUsernames(
14+
const targetAccounts = await SubaccountUsernamesTable.getSubaccountZerosWithoutUsernames(
1815
config.SUBACCOUNT_USERNAME_BATCH_SIZE,
1916
);
2017
stats.timing(
@@ -26,7 +23,7 @@ export default async function runTask(): Promise<void> {
2623
const txnStart: number = Date.now();
2724
try {
2825
let successCount: number = 0;
29-
for (const subaccount of subaccountZerosWithoutUsername) {
26+
for (const subaccount of targetAccounts) {
3027
for (let i = 0; i < config.ATTEMPT_PER_SUBACCOUNT; i++) {
3128
const username: string = generateUsernameForSubaccount(
3229
subaccount.address,
@@ -38,43 +35,42 @@ export default async function runTask(): Promise<void> {
3835
i,
3936
);
4037
try {
41-
await SubaccountUsernamesTable.create({
38+
const count: number = await SubaccountUsernamesTable.insertAndReturnCount(
4239
username,
43-
subaccountId: subaccount.subaccountId,
44-
}, { txId });
45-
// If success, break from loop and move to next subaccount.
46-
successCount += 1;
47-
break;
48-
} catch (e) {
49-
// There are roughly ~225 million possible usernames
50-
// so the chance of collision is very low.
51-
if (e instanceof Error && e.name === 'UniqueViolationError') {
52-
stats.increment(
53-
`${config.SERVICE_NAME}.subaccount-username-generator.collision`, 1);
54-
logger.info({
55-
at: 'subaccount-username-generator#runTask',
56-
message: 'username collision',
57-
address: subaccount.address,
58-
subaccountId: subaccount.subaccountId,
59-
username,
60-
error: e,
61-
});
40+
subaccount.subaccountId,
41+
{ txId },
42+
);
43+
if (count > 0) {
44+
successCount += 1;
45+
break;
6246
} else {
47+
// if count is 0, log error and continue to next iteration
48+
// which will bump the nonce and try again with a new username
6349
logger.error({
6450
at: 'subaccount-username-generator#runTask',
6551
message: 'Failed to insert username for subaccount',
6652
address: subaccount.address,
6753
subaccountId: subaccount.subaccountId,
6854
username,
69-
error: e,
55+
error: new Error('Username already exists'),
7056
});
7157
}
58+
} catch (e) {
59+
logger.error({
60+
at: 'subaccount-username-generator#runTask',
61+
message: 'Failed to insert username for subaccount',
62+
address: subaccount.address,
63+
subaccountId: subaccount.subaccountId,
64+
username,
65+
error: e,
66+
});
67+
throw e;
7268
}
7369
}
7470
}
7571
await Transaction.commit(txId);
7672
const subaccountAddresses = _.map(
77-
subaccountZerosWithoutUsername,
73+
targetAccounts,
7874
(subaccount) => subaccount.address,
7975
);
8076
stats.timing(
@@ -84,7 +80,7 @@ export default async function runTask(): Promise<void> {
8480
logger.info({
8581
at: 'subaccount-username-generator#runTask',
8682
message: 'Generated usernames',
87-
batchSize: subaccountZerosWithoutUsername.length,
83+
batchSize: targetAccounts.length,
8884
successCount,
8985
addressSample: subaccountAddresses.slice(0, 10),
9086
duration: Date.now() - taskStart,
@@ -93,7 +89,7 @@ export default async function runTask(): Promise<void> {
9389
await Transaction.rollback(txId);
9490
logger.error({
9591
at: 'subaccount-username-generator#runTask',
96-
message: 'Error when updating totalVolume in wallets table',
92+
message: 'Error when generating usernames for subaccounts',
9793
error,
9894
});
9995
}

0 commit comments

Comments
 (0)