feat(api-kit): 4337 API functions #777
Conversation
Pull Request Test Coverage Report for Build 9082904763Warning: 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 |
packages/api-kit/src/SafeApiKit.ts
Outdated
| * @throws "Invalid Ethereum address {safeAddress}" | ||
| * @returns The SafeOperations sent from the given Safe's address | ||
| */ | ||
| async getSafeOperationsByAddress(safeAddress: string): Promise<GetSafeOperationListResponse> { |
There was a problem hiding this comment.
We seem to be ignoring the paging parameters (offset, limit etc) while the response does have the full result (count, next). Is this deliberate?
There was a problem hiding this comment.
Was not on purpose 😅 added the missing params now
packages/api-kit/src/utils/index.ts
Outdated
| @@ -0,0 +1,3 @@ | |||
| import { EMPTY_DATA } from './constants' | |||
|
|
|||
| export const isEmptyHexData = (input: string) => !input || input === EMPTY_DATA | |||
There was a problem hiding this comment.
I would move the !input outside of the function or rename it since empty hex implies a hex.
| callGasLimit?: bigint | ||
| } | ||
|
|
||
| export interface SafeOperation { |
There was a problem hiding this comment.
It might be a good idea to avoid name colision even if this is scoped. People often use ctrl+shift+f to look for definitions.
packages/api-kit/src/SafeApiKit.ts
Outdated
| async addSafeOperation({ | ||
| moduleAddress: moduleAddressProp, | ||
| safeAddress: safeAddressProp, | ||
| safeOperation, |
There was a problem hiding this comment.
So... I dont understand the use case for this particular function but here is some food for thought. Right now, there is no internal coupling between this and the Safe4337 module which is better then if we were importing the type from there. There is, however, an implicit one. For someone to use this endpoint, they would have to either: use Safe4337Pack#createTransaction or implementing an interface themselves that accomplishes the same thing (which is a bit awkward). Are there situation where all these things could be supplied by an external library or people should ALWAYS use both modules together? Should we really use such complex object just for an api proxy and should the api-kit itself sign in this case? Perhaps the interface here is just really an UserOperation (which is clearly defined by the ERC) and the fields associated with packing signature (validUntil, validAfter and the authorizer's signature). That data field also almost look like something to represent the internal state of the object.
There was a problem hiding this comment.
I refactored the function in 56315ef to not expect a SafeOperation anymore, but a UserOperation object.
packages/api-kit/src/SafeApiKit.ts
Outdated
| } | ||
|
|
||
| const signerAddress = await signer.getAddress() | ||
| const signature = safeOperation.getSignature(signerAddress) |
There was a problem hiding this comment.
I was looking at the server-side code and this field seems to be validated like this:
safe_owners = self._get_owners(safe_address)
parsed_signatures = SafeSignature.parse_signature(
signature, safe_operation_hash, safe_operation_hash_preimage
)
owners_processed = set()
safe_signatures = []
for safe_signature in parsed_signatures:
# do stuff
Please just check if those are not every owner's concatenated signature. I think the SafeOperation#encodedSignatures() or something.
…ng with Safe4337 module The function expects a `UserOperation` object now, instead of a `SafeOperation` object.
Remove unused PIMLICO_API_KEY Co-authored-by: Daniel <[email protected]>
tmjssz
force-pushed
the
feat/4337-api-integration
branch
from
691cca2 to
cd124d8
Compare
…e-sdk into feat/4337-api-integration # Conflicts: # packages/api-kit/tests/endpoint/index.test.ts # packages/relay-kit/src/packs/safe-4337/types.ts
This is for better differentiation with the corresponding interface.
What it solves
Resolves #775
Adding support for the following 4337 API endpoints:
/v1/safe-operations/{safe_operation_hash}//v1/safes/{address}/safe-operations//v1/safes/{address}/safe-operations/