Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(api-kit): 4337 API functions #777
feat(api-kit): 4337 API functions #777
Changes from 8 commits
10e6907
aed236b
f4ad53d
9c49f60
5153baa
ba4937e
a9e0a66
4640747
f0846b4
295b68b
2097ef0
236d0fc
483d90b
83f494e
b02b85e
46fd0c4
56315ef
05054d3
cd124d8
18d0c84
9a4ad4f
29b6cd1
8099ed4
6ac7ea9
26142c1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 116 in packages/api-kit/src/SafeApiKit.ts
GitHub Actions / eslint
Check warning on line 326 in packages/api-kit/src/SafeApiKit.ts
GitHub Actions / eslint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was not on purpose 😅 added the missing params now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 anUserOperation
(which is clearly defined by the ERC) and the fields associated with packing signature (validUntil
,validAfter
and the authorizer'ssignature
). That data field also almost look like something to represent the internal state of the object.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored the function in 56315ef to not expect a
SafeOperation
anymore, but aUserOperation
object.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking at the server-side code and this field seems to be validated like this:
Please just check if those are not every owner's concatenated signature. I think the
SafeOperation#encodedSignatures()
or something.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move the
!input
outside of the function or rename it since empty hex implies a hex.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed the function