feat(api-kit): Support for SafeOperation confirmations endpoints #876
Conversation
Calls the transaction-service's `POST /v1/safe-operations/{safe_operation_hash}/confirmations/` endpoint.
Calls the transaction-service's `GET /v1/safe-operations/{safe_operation_hash}/confirmations/` endpoint.
Fix endpoint test for `confirmTransaction`
Pull Request Test Coverage Report for Build 9592708241Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9592817524Details
💛 - Coveralls |
| @@ -1,4 +1,4 @@ | |||
| import SafeApiKit from '@safe-global/api-kit' | |||
| import SafeApiKit from '@safe-global/api-kit/index' | |||
There was a problem hiding this comment.
Is it mandatory to add the index?
There was a problem hiding this comment.
Without it, I see this weird error:
All the other files in the same folder are defining the same import path for the SafeApiKit. That's why I changed it accordingly here and it removed the error.
Pull Request Test Coverage Report for Build 9599977083Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
Some tests related to SafeDelegates started to fail, but it seems unrelated to my changes because I'm getting the same errors on the development branch 
|
|
Don't worry about the tests, they are not related with this PR. Already checked with the API team. Related to this #879 |
Pull Request Test Coverage Report for Build 9614186193Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9646217403Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
| @@ -325,3 +286,12 @@ export type AddSafeOperationProps = { | |||
| validAfter?: number | |||
| } | |||
| } | |||
|
|
|||
| export type GetSafeOperationConfirmationListProps = { | |||
There was a problem hiding this comment.
It is a little bit weird to use limit and offset as a string since they are associated with record count in a database. I imagine that the interface will be something like apiKit.getSafeOperationConfirmations({ safeOperationHash, limit: '10' }).
There was a problem hiding this comment.
Yes good point. Changed the type to number
packages/api-kit/src/SafeApiKit.ts
Outdated
| * @throws "Invalid SafeOperation hash" | ||
| * @throws "Invalid data" | ||
| */ | ||
| async getSafeOperationConfirmations({ |
There was a problem hiding this comment.
Please double check how we do this. This kind of looks like a getSafeOperationConfirmationsByHash with a different name as the hash itself is a part of a "perma-link". In this case, it would be common to separate the interface as getSafeOperationConfirmations(hash, { limit, offset }) since that would be somehow associated with a domain model.
There was a problem hiding this comment.
Updated the function signature accordingly 👍
Pull Request Test Coverage Report for Build 9666618943Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
This changes the type for `limit` + `offset` from string to number for the functions: * getSafeDelegates * getMessages * getSafeOperationsByAddress
Pull Request Test Coverage Report for Build 9676087509Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
What it solves
Resolves #856
How this PR fixes it
Extend the
api-kitby two functions using the following endpoints of the transaction-service.getSafeOperationConfirmations
Returns the list of confirmations for a given a SafeOperation.
confirmSafeOperation
Adds a confirmation for a SafeOperation.