Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Migrate MultisigTransaction to zod #1385

Merged
merged 3 commits into from
Apr 23, 2024
Merged

Migrate MultisigTransaction to zod #1385

merged 3 commits into from
Apr 23, 2024

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Apr 9, 2024 •
edited
Loading

Summary

This migrates the validation of MultisigTransactions to zod.

Changes

  • Replace confirmationSchema with ConfirmationSchema
  • Replace multisigTransactionSchema with MultisigTransactionSchema
  • Remove multisigTransactionPageSchema
  • Infer types from the schemas and propegate them.
  • Add appropriate test coverage for the above

@iamacook iamacook self-assigned this Apr 9, 2024
iamacook
iamacook commented Apr 9, 2024
View reviewed changes
src/datasources/transaction-api/transaction-api.service.spec.ts
@@ -1301,7 +1301,7 @@ describe('TransactionApi', () => {
executionDateGte: executedDateGte,
executionDateLte: executedDateLte,
to: multisigTransaction.to,
value: multisigTransaction.value,
value: multisigTransaction.value ?? undefined,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is purely used to satisfy TypeScript.

iamacook
iamacook commented Apr 9, 2024
View reviewed changes
src/domain/messages/entities/message-confirmation.entity.ts
@@ -1,6 +1,7 @@
import { MessageConfirmationSchema } from '@/domain/messages/entities/schemas/message.schema';
import { z } from 'zod';

// TODO: Move to a shared location as used for other entities
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't do this to reduce the footprint of the PR.

iamacook
iamacook commented Apr 9, 2024
View reviewed changes
src/domain/safe/transaction-type.validator.ts
Comment on lines +94 to +98
this.jsonSchemaService.getSchema(
CONFIRMATION_SCHEMA_ID,
confirmationSchema,
);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is temporarily needed until we migrate the transactionTypePageSchema.

iamacook
iamacook commented Apr 9, 2024
View reviewed changes
src/routes/transactions/entities/transaction-data.entity.ts
@@ -28,7 +28,7 @@ export class TransactionData {
hexData: string | null,
dataDecoded: DataDecoded | null,
to: AddressInfo,
value: string,
value: string | null,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we weren't inferring the types from the schema, this was not caught. It matches the types of the Transaction Service.

iamacook
iamacook commented Apr 9, 2024
View reviewed changes
src/routes/transactions/mappers/multisig-transactions/multisig-transaction-status.mapper.ts Outdated
@@ -19,7 +19,7 @@ export class MultisigTransactionStatusMapper {
}
if (
(transaction.confirmations?.length || 0) <
transaction.confirmationsRequired
(transaction.confirmationsRequired || 0)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As before regarding matching the Transaction Service.

@iamacook iamacook marked this pull request as ready for review April 9, 2024 15:19
@iamacook iamacook requested a review from a team as a code owner April 9, 2024 15:19
@coveralls
Copy link

coveralls commented Apr 9, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 8786536293

Details

  • 19 of 19 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 92.981%
Totals Coverage Status
Change from base Build 8658173764: -0.03%
Covered Lines: 6758
Relevant Lines: 7010
💛 - Coveralls

@iamacook iamacook mentioned this pull request Apr 12, 2024
Merged
@fmrsabino fmrsabino added in review Someone is reviewing this Pull Request and removed in review Someone is reviewing this Pull Request labels Apr 15, 2024
@fmrsabino fmrsabino added the in review Someone is reviewing this Pull Request label Apr 22, 2024
fmrsabino
fmrsabino reviewed Apr 22, 2024
View reviewed changes
src/domain/safe/entities/schemas/multisig-transaction.schema.ts Outdated Show resolved Hide resolved
src/domain/safe/entities/schemas/multisig-transaction.schema.ts Outdated
signature: HexSchema.nullish().default(null),
});

export const MultisigTransactionSchema = z.object({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The multisigTransactionPageSchema and MULTISIG_TRANSACTION_PAGE_SCHEMA_ID are no longer needed right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I've removed them in d8857aa.

...ons/__tests__/controllers/list-multisig-transactions-by-safe.transactions.controller.spec.ts Outdated Show resolved Hide resolved
src/routes/transactions/entities/multisig-execution-info.entity.ts Outdated Show resolved Hide resolved
@fmrsabino fmrsabino removed the in review Someone is reviewing this Pull Request label Apr 22, 2024
@iamacook
Copy link
Member Author

If count is nullable why did we have to change the count from null to 1 here? 🤔

It won't let me respond to this. It was a poor invalid value. It won't validate a string. I changed it to "invalid" in d8857aa for clarity.

@iamacook iamacook requested a review from fmrsabino April 22, 2024 15:18
@fmrsabino
Copy link
Contributor

If count is nullable why did we have to change the count from null to 1 here? 🤔

It won't let me respond to this. It was a poor invalid value. It won't validate a string. I changed it to "invalid" in d8857aa for clarity.

Oh I see. Thanks for clarifying! Now I understand that it was the difference between a valid number vs a numeric string 👍 Thanks!

fmrsabino
fmrsabino approved these changes Apr 22, 2024
View reviewed changes
Copy link
Contributor

@fmrsabino fmrsabino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 👏 👏

@iamacook iamacook merged commit 2a822e2 into main Apr 23, 2024
16 checks passed
@iamacook iamacook deleted the multisig-transaction-zod branch April 23, 2024 07:09
@iamacook iamacook mentioned this pull request Apr 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmrsabino fmrsabino fmrsabino approved these changes

Assignees

@iamacook iamacook

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@iamacook @coveralls @fmrsabino