Add name property to Accounts #2081
Conversation
| const hash = crypto.createHash('sha256'); | ||
| hash.update(name); | ||
| const nameHash = hash.digest('hex'); | ||
| // TODO: encrypt the name |
There was a problem hiding this comment.
The actual name encryption will be added in a forthcoming PR, along with AWS KMS support, which is needed to encrypt the name.
34ab268 to
cfc7732
Compare
| }), | ||
| ]), | ||
| ); | ||
| }); | ||
|
|
||
| it.todo('should fail if the name already exists'); |
| // Note: regular expression is simplified because faker has limited support for regex. | ||
| // https://fakerjs.dev/api/helpers#fromregexp | ||
| .with('name', faker.helpers.fromRegExp(/[a-zA-Z0-9]{12,20}/i)) |
There was a problem hiding this comment.
Nit: what do you think about extracting this considering it's also used in the accountBuilder? Otherwise we could use one of the word faker methods?
There was a problem hiding this comment.
I've extracted it in 2069930
| name: z | ||
| .string() | ||
| .min(3, { message: 'Account names must be at least 3 characters long' }) | ||
| .max(20, { message: 'Account names must be at most 20 characters long' }) | ||
| .regex(/^[a-zA-Z0-9]+(?:[._-][a-zA-Z0-9]+)*$/, { | ||
| message: | ||
| 'Account names must start with a letter or number and can contain alphanumeric characters, periods, underscores, and hyphens', | ||
| }), | ||
| }); |
There was a problem hiding this comment.
As before regarding extracting. We could have a AccountNameSchema, etc. that is tested and extended for this and the AccountSchema.
| @@ -68,4 +75,8 @@ describe('CreateAccountDtoSchema', () => { | |||
| ); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('name', () => { | |||
| // TODO: Add tests for name validation | |||
| }), | ||
| ]), | ||
| ); | ||
| }); | ||
|
|
||
| it('should fail if the name already exists', async () => { |
There was a problem hiding this comment.
Should we also check that the name hashes are unique?
There was a problem hiding this comment.
I totally agree, but I think we cannot test that at the datasource level as the hash is not included in the function input parameters, it is calculated internally. I've added a test to verify the hashes are unique (the uniqueness constraint is applied) to the migration tests (00009_account-names.spec.ts) in 30b3e87
src/domain/accounts/entities/schemas/account-name.schema.spec.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Aaron Cook <aaron@safe.global>
| export function accountBuilder(): IBuilder<Account> { | ||
| return new Builder<Account>() | ||
| .with('id', faker.number.int({ max: DB_MAX_SAFE_INTEGER })) | ||
| .with('group_id', faker.number.int({ max: DB_MAX_SAFE_INTEGER })) | ||
| .with('address', getAddress(faker.finance.ethereumAddress())) | ||
| .with('name', faker.helpers.fromRegExp(accountNameSimpleRegex)) |
There was a problem hiding this comment.
Nit: what do you think about creating a specific builder for this and using that everywhere to ensure the fromRegexp is used?
Summary
This PR adds the
nameproperty to theAccountentity. As this property is intended to be encrypted at the database level, and a unique constraint should also be applied to that column, an additionalname_hashis added. The unique constraint is applied to thename_hash, so we can ensurenamecannot contain duplicates once they are decrypted (thenamevalues cannot be compared directly as they are encrypted).Changes
nameandname_hashcolumns to theaccountstable.nameproperty to theAccountentity.namefield.