Feat: add CGW Accounts API specification #190
Conversation
src/types/accounts.ts
Outdated
| name: string | ||
| description: string | null | ||
| isActive: boolean | ||
| } | ||
|
|
||
| export type AccountDataSetting = { | ||
| name: string | ||
| description: string | null | ||
| enabled: boolean |
There was a problem hiding this comment.
There seems to be an overlap here. Can we combine this into a shared type?
Also is isActive and enabled the same thing?
There was a problem hiding this comment.
isActiveis an AccountDataType-only property. It indicates if the DataType (CF Safes, watchlist, etc.) is generally available in the CGW. The idea behind this is to provide a list of available DataTypes so the UI could build the list of checkboxes (or another widget) for the user to select using this as a source of truth. An alternative approach would be to use thisisActiveinternally in the CGW and only return the active ones to the UI. This would be simpler but also less transparent. Do you have any preference regarding this? 🙂enabledis an AccountDataSetting-only property. It represents a specific user setting (i.e.: whether the checkbox is marked or it isn't)
Regarding the overlap you commented on, I think you are right and this API can also be simplified. I'm working in the CGW on that regard and will update this accordingly later on 🙂
There was a problem hiding this comment.
I've modified the API on the CGW side, and I have also modified this PR accordingly in eaa943b
Account.accountIdwas renamed to simplyid(as it's the type main identifier).- I've solved the overlap between the
AccountDataTypeandAccountDataSettingtypes.
src/types/accounts.ts
Outdated
| enabled: boolean | ||
| } | ||
|
|
||
| export type CreateAccountDto = { |
There was a problem hiding this comment.
Please don't use the DTO acronym in type names. It makes sense within the internal CGW architecture, but for clients it's a non-concept.
src/types/accounts.ts
Outdated
| accountDataSettings: { | ||
| id: string | ||
| enabled: boolean |
There was a problem hiding this comment.
Another (partial) duplication of AccountDataSetting?
There was a problem hiding this comment.
Also solved in eaa943b
Co-authored-by: Usame Algan <5880855+usame-algan@users.noreply.github.com>
Co-authored-by: Usame Algan <5880855+usame-algan@users.noreply.github.com>
Co-authored-by: Usame Algan <5880855+usame-algan@users.noreply.github.com>
| '/v1/accounts/{address}/data-settings': { | ||
| get: operations['get_account_data_settings'] | ||
| put: operations['put_account_data_settings'] | ||
| parameters: { | ||
| path: { | ||
| address: string | ||
| } | ||
| credentials: 'include' | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure if we have to define the body here as well but when calling putAccountDataSettings there is no type checking so I can pass anything I want as the second parameter in putAccountDataSettings(wallet.address, {}) without an error 🤔 @katspaugh do you remember if this is expected?
hectorgomezv
changed the title
|
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2024 Safe Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
Changes
Account,AccountDataType,AcocuntDataSetting,CreateAccountDto,UpsertAccountDataSettingsDto.POST /v1/accountsGET /v1/accounts/{address}DELETE /v1/accounts/{address}GET /v1/accounts/data-typesGET /v1/accounts/{address}/data-settingsPUT /v1/accounts/{address}/data-settings