-
Notifications
You must be signed in to change notification settings - Fork 133
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: add useSendUserOperation and useDropAndReplaceUserOperation hooks #581
feat: add useSendUserOperation and useDropAndReplaceUserOperation hooks #581
Conversation
2e793e0
to
de3c028
Compare
if (!client) { | ||
throw new ClientUndefinedError("useDropAndReplaceUserOperation"); | ||
} |
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.
ditto comment made in another hook pr:
or all out hooks, we should check from AlchemyAccountContext which state the user/account is in, and throw informative error messages instead of just throwing null exceptions or other generic error messages
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.
See my comment in the use client actions hook about this. I don't think this is a generic error. There's nothing to check in the account context that will make it more informative. The client here is a parameter to the hook and can be null. If it's null we error. Beyond that what else can we pull from context? What if the user is passing in a client they didn't create via context and it's still null?
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.
you are right. I left a similar comment on signer hook prs, and left similar comments here, but I realized that the core SDK takes care of these already.
error: Error | null; | ||
}; | ||
|
||
export function useSendUserOperation< |
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.
this is only meant to be used for smart accounts? we should check if account is connected to the client, not just that client exists. in general, throw more informative state error back to the caller.
Makes sense to group signer/smart account/modular account hooks separately in the subdirs of hooks for different accounts, and also have more common state checking/error messages/code shared.
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.
The client will throw if it is not connected to an account already and the typing on this specific client parameter forbids it from not not being connected anyways since it has to be a LA client or MA client
6eb9f4f
to
6a67fd0
Compare
1ee77cd
to
11d90d9
Compare
42cce3c
to
8cc8e08
Compare
13253a7
to
caa5475
Compare
8cc8e08
to
82f9f13
Compare
caa5475
to
9a0cd70
Compare
9a0cd70
to
4090d5d
Compare
Pull Request Checklist
yarn test
)site
folder, and guidelines for updating/adding docs can be found in the contribution guide)feat!: breaking change
)yarn lint:check
) and fix any issues? (yarn lint:write
)PR-Codex overview
This PR adds new hooks
useSendUserOperation
anduseDropAndReplaceUserOperation
for interacting with Alchemy smart accounts in React components.Detailed summary
NoAlchemyAccountContextError
classuseSendUserOperation
anduseDropAndReplaceUserOperation
hooksalchemy/src/react/index.ts