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.

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

Add transaction deletion endpoint #1063

Merged
merged 3 commits into from
Jan 25, 2024
Merged

Conversation

iamacook
Copy link
Member

This adds the DELETE chains/:chainId/transactions/:safeTxHash endpoint for deleting transactions (in accordance with the DELETE /v1/transactions/{safe_tx_hash} endpoint of the Transaction Service) with associated test coverage.

Changes

From "root" to call:

  • deleteTransaction has been added to the TransactionApi and ITransactionApi interface
  • deleteTransaction has been added to the SafeRepository and ISafeRepository interface
  • deleteTransaction has been added to the TransactionsService
  • DELETE chains/:chainId/transactions/:safeTxHash has been added to the TransactionsController, accepting the DeleteTransactionDto (and validated by the DeleteTransactionDtoValidationPipe)

DeleteTransactionDto

 {
    "signature": signature,
 }

According to the Transaction Service, the above is used as follows:

An EOA is required to sign the following EIP712 data:

 {
    "types": {
        "EIP712Domain": [
            {"name": "name", "type": "string"},
            {"name": "version", "type": "string"},
            {"name": "chainId", "type": "uint256"},
            {"name": "verifyingContract", "type": "address"},
        ],
        "DeleteRequest": [
            {"name": "safeTxHash", "type": "bytes32"},
            {"name": "totp", "type": "uint256"},
        ],
    },
    "primaryType": "DeleteRequest",
    "domain": {
        "name": "Safe Transaction Service",
        "version": "1.0",
        "chainId": chain_id,
        "verifyingContract": safe_address,
    },
    "message": {
        "safeTxHash": safe_tx_hash,
        "totp": totp,
    },
}

The totp parameter is calculated with T0=0 and Tx=3600. It is calculated by taking the Unix UTC epoch time (without milliseconds) and dividing it by 3600 (natural division, no decimals).

@iamacook iamacook self-assigned this Jan 24, 2024
@iamacook iamacook requested a review from a team as a code owner January 24, 2024 09:15
@hectorgomezv hectorgomezv added the in review Someone is reviewing this Pull Request label Jan 24, 2024
Copy link
Member

@hectorgomezv hectorgomezv left a comment

Choose a reason for hiding this comment

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

Great job! 👏 I've left comments related to minor things only.

DELETE_TRANSACTION_DTO_SCHEMA_ID,
deleteTransactionDtoSchema,
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: missing newline between functions 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in 731824e.

);
}
transform(data: unknown): DeleteTransactionDto {
return this.genericValidator.validate(this.isValid, data);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should set the error status (400 - bad request) here, what do you think?

It's done that way in other ValidationPipes and I think it makes sense to communicate that a validation error at this level is a client-side error (400).

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I've added it in 731824e but in a type safe way (adjusted in other PRs).

@hectorgomezv hectorgomezv removed the in review Someone is reviewing this Pull Request label Jan 24, 2024
@hectorgomezv hectorgomezv self-requested a review January 25, 2024 10:23
@iamacook iamacook merged commit b960f36 into main Jan 25, 2024
16 checks passed
@iamacook iamacook deleted the delete-transaction-endpoint branch January 25, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants