[TRA-571] Add vault table and vault event version/subtype. #2263
Conversation
WalkthroughThe changes introduce a comprehensive vault management system within a PostgreSQL-based application. This includes new constants, a migration for creating a Changes
Possibly related PRs
Suggested labels
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- indexer/packages/postgres/tests/helpers/constants.ts (2 hunks)
- indexer/packages/postgres/tests/stores/vault-table.test.ts (1 hunks)
- indexer/packages/postgres/src/db/migrations/migration_files/20240912180829_create_vaults_table.ts (1 hunks)
- indexer/packages/postgres/src/helpers/db-helpers.ts (1 hunks)
- indexer/packages/postgres/src/models/vault-model.ts (1 hunks)
- indexer/packages/postgres/src/stores/vault-table.ts (1 hunks)
- indexer/packages/postgres/src/types/db-model-types.ts (2 hunks)
- indexer/packages/postgres/src/types/index.ts (1 hunks)
- indexer/packages/postgres/src/types/query-types.ts (1 hunks)
- indexer/packages/postgres/src/types/vault-types.ts (1 hunks)
- protocol/indexer/events/constants.go (3 hunks)
Additional comments not posted (27)
indexer/packages/postgres/src/types/vault-types.ts (3)
3-9: LGTM!The
VaultCreateObjectinterface provides a clear and well-structured definition for creating a vault. The property names are descriptive, and their types are appropriately defined. The use of theVaultStatusenum for thestatusproperty ensures type safety and clarity. Additionally, theIsoStringtype is correctly used for thecreatedAtandupdatedAtproperties to represent ISO 8601 formatted date strings.
11-16: LGTM!The
VaultStatusenum provides a clear and well-defined set of possible states for a vault. The state names are descriptive and follow a consistent naming convention (uppercase with underscores). These states likely represent different operational modes or conditions of the vault, enhancing the clarity and maintainability of the codebase.
18-24: LGTM!The
VaultColumnsenum is a helpful addition that provides string constants for the property names of theVaultCreateObjectinterface. Using an enum for column names is a good practice for ensuring consistency and avoiding typos when referencing these properties in other parts of the codebase, such as database queries or data manipulation. This enhances the maintainability and reliability of the code.indexer/packages/postgres/src/db/migrations/migration_files/20240912180829_create_vaults_table.ts (2)
3-16: LGTM!The
upmigration function correctly creates thevaultstable with appropriate columns and constraints. The chosen column types and thestatusenum values align well with the intended purpose of storing vault information.Including the
createdAtandupdatedAttimestamps is a good practice for tracking vault creation and modification.
18-20: LGTM!The
downmigration function correctly drops thevaultstable, which is the appropriate way to revert the migration created by theupfunction.indexer/packages/postgres/src/models/vault-model.ts (9)
1-3: LGTM!The imports are relevant and necessary for the class implementation.
5-13: LGTM!The
VaultModelclass correctly extends theBaseModelclass. The static methodstableNameandidColumnare properly defined.
15-33: LGTM!The
jsonSchemamethod correctly defines the structure and validation rules for the vaults. The required properties, types, and formats are properly specified.
35-36: LGTM!The
addressproperty is correctly defined as a non-nullable string type.
37-38: LGTM!The
clobPairIdproperty is correctly defined as a non-nullable string type.
39-40: LGTM!The
statusproperty is correctly defined as a non-nullableVaultStatustype.
41-42: LGTM!The
createdAtproperty is correctly defined as a non-nullableIsoStringtype.
43-44: LGTM!The
updatedAtproperty is correctly defined as a non-nullableIsoStringtype.
5-44: LGTM!The
VaultModelclass is well-structured and follows the necessary conventions for a data model. The static methods and properties are properly defined and typed. The JSON schema correctly defines the structure and validation rules for the vaults. The class provides a clear and consistent representation of the vaults data.indexer/packages/postgres/src/types/index.ts (1)
35-35: LGTM!The export statement follows the existing pattern and makes the vault types accessible to other parts of the codebase. This change enhances the module's interface by incorporating additional types related to vaults.
Note: Ensure that the exported vault types are used correctly and consistently throughout the codebase to maintain type safety and avoid any potential issues.
protocol/indexer/events/constants.go (2)
23-23: LGTM!The addition of the new constant
SubtypeUpsertVaultis in line with the PR objective and follows the existing naming convention.
43-43: LGTM!The addition of the new constant
UpsertVaultEventVersionand the inclusion ofSubtypeUpsertVaultin theOnChainEventSubtypesarray are consistent with the PR objective and the addition of the new subtype. The initial version is set correctly to1.Also applies to: 61-61
indexer/packages/postgres/__tests__/stores/vault-table.test.ts (2)
27-50: LGTM!The test case is well-structured and covers the essential aspects of the
findAllmethod. It ensures that all created vaults are retrieved correctly with the expected data.
52-80: LGTM!The test case thoroughly verifies the
upsertmethod by:
- Creating a vault and verifying its data.
- Upserting the vault with updated status and verifying the changes.
It ensures that the
upsertmethod updates the vault data correctly while keeping the other data intact.indexer/packages/postgres/src/stores/vault-table.ts (3)
21-77: LGTM!The
findAllfunction is well-structured and follows best practices for querying a database using an ORM. The use of optional filters and pagination allows for flexibility in querying the database. The function correctly verifies the presence of required fields before constructing the base query, and the base query is modified according to the provided filters and sorting options, which is a good practice. The default sorting order is set toclobPairIdin ascending order, which is a reasonable default. The function returns all matching records, which is the expected behavior.
79-88: LGTM!The
createfunction is a simple wrapper around the Objection.jsinsertmethod. It correctly spreads thevaultToCreateobject into theinsertmethod, which is a good practice. The function returns the created vault record, which is the expected behavior.
90-100: LGTM!The
upsertfunction is a simple wrapper around the Objection.jsupsertmethod. It correctly spreads thevaultToUpsertobject into theupsertmethod, which is a good practice. The function returns the first vault from the resulting array of records, which is the expected behavior.indexer/packages/postgres/src/helpers/db-helpers.ts (1)
34-34: LGTM!The addition of the
'vaults'entry to thelayer1Tablesarray is consistent with the PR objective of creating a vault table within the Indexer. This change ensures that thevaultstable is included when deleting data using thedropDataorclearDatafunctions.indexer/packages/postgres/src/types/db-model-types.ts (1)
305-311: LGTM!The new
VaultFromDatabaseinterface provides a clear and well-structured schema for vault entities in the database. The included properties are relevant and properly typed.Exporting the interface makes it accessible for use in other parts of the codebase, enabling consistent handling of vault data across the system.
The changes enhance the data model without altering existing functionality, effectively accommodating new requirements related to vault management.
indexer/packages/postgres/src/types/query-types.ts (1)
348-352: LGTM!The new
VaultQueryConfiginterface follows the existing pattern and enhances the querying capabilities for vaults. The optional properties allow for flexible filtering based on multiple addresses, CLOB pair IDs, and statuses.indexer/packages/postgres/__tests__/helpers/constants.ts (2)
63-64: LGTM!The import statement for
VaultCreateObjectandVaultStatustypes is correctly placed and will likely be used in the subsequent code changes.
999-1007: LGTM!The new constants
defaultVaultAddressanddefaultVaultare implemented correctly:
- The constants follow the naming convention and are exported for use in tests.
- The
defaultVaultobject is created with reasonable default values, including the correct usage of theVaultStatusenum.These additions will be helpful for testing vault-related functionality.
| it('Successfully creates a vault', async () => { | ||
| await VaultTable.create(defaultVault); | ||
| }); |
There was a problem hiding this comment.
Add assertions to verify the vault creation.
The test case ensures that the create method can be called without errors. However, it does not verify that the vault is actually created with the correct data.
Consider adding assertions to verify that the created vault matches the input data:
it('Successfully creates a vault', async () => {
await VaultTable.create(defaultVault);
+ const createdVault = await VaultTable.findAll({}, [], { readReplica: true });
+ expect(createdVault.length).toEqual(1);
+ expect(createdVault[0]).toEqual(expect.objectContaining(defaultVault));
});Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('Successfully creates a vault', async () => { | |
| await VaultTable.create(defaultVault); | |
| }); | |
| it('Successfully creates a vault', async () => { | |
| await VaultTable.create(defaultVault); | |
| const createdVault = await VaultTable.findAll({}, [], { readReplica: true }); | |
| expect(createdVault.length).toEqual(1); | |
| expect(createdVault[0]).toEqual(expect.objectContaining(defaultVault)); | |
| }); |
| @@ -0,0 +1,20 @@ | |||
| import * as Knex from 'knex'; | |||
|
|
|||
| export async function up(knex: Knex): Promise<void> { | |||
There was a problem hiding this comment.
for my understanding, this logic is run when indexer is upgraded?
|
Indexer does not need to know about decommissioning of a vault, as the subaccount / balances for the vault will still exist. |
Changelist
Add subtype for vault event + vault table in Indexer.
Test Plan
Unit tests.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests