[react]: add useSendTransaction hook#229
Conversation
This reverts commit 4fa027d.
…into useSendTransaction
🦋 Changeset detectedLatest commit: d9dce41 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@nickfrosty check this out once (my bad multiple commits in order to fix the changeset file) |
|
What's the benefit of using a mutation over something like |
|
@macalinao here we used |
GuiBibeau
left a comment
There was a problem hiding this comment.
Hey! Nice work on this hook 🎉
Just noticed a small naming issue: the parameter signature in line 19 should probably be wireTransaction or transaction instead.
Currently it's a bit confusing because:
- We're passing IN a transaction (base64 encoded)
- We're getting OUT a signature (the transaction ID)
Using signature for the input makes it seem like we're passing in a signature, when we're actually passing in the whole transaction. What do you think about renaming it?
|
@GuiBibeau thanks for the review i'll make sure all the changes which are reviewed will be added by tomorrow |
There was a problem hiding this comment.
The idea for this hook is good but the usage pattern is wrong...
Think about how a dev would actually use this hook as its currently written: you have to have the transaction already before even instantiating the hook. Which means you could only really uses this inside a guard component that has the transaction passed to it. That makes no sense.
Instead, developer usage should look something like this:
const { sendTransaction } = useSendTransaction({
config: {
encoding: "base64",
preflightCommitment: "confirmed",
skipPreflight: false,
},
});
const signature = await sendTransaction(...validTransaction);Please rework this hook to:
- rebase to master
- reword the hook as described above
- fix the imports in the files you added to use
.jsfile extensions (see the other hooks and typeset tests) - fix the incorrect readme entry (that does not even match how you would use this as currently written) to be correct for the updated usage
- remove your retry delay since it does not give users any flexibility of control
- remove your incorrect comment for
return the transaction signature as a base-64 encoded string. its aSignature, which is a base58 encoded string
edit: this hook also needs to support the options and config, similarly to how all the other hooks accept them.
|
@nickfrosty I'm doing all the changes you mentioned. questions: the edit you mentioned about the thought: it is good to have them globally by defining them in the |
tobeycodes
left a comment
There was a problem hiding this comment.
It looks like no commits have been made since this comment so this is still a work in progress.
|
@tobeycodes #229 (comment) for this question can you give the clarity for me in this the types mentioned there are only for the useQuery hook they are not working for the useMutation which we are using in the sendTransaction. Thinking: to add these types in the sendTransaction hook itself. i need the clarification on this. |
tobeycodes
left a comment
There was a problem hiding this comment.
Nice work.
question: can you give me an example when you would use sendTransaction instead of signAndSendTransaction?
tobeycodes
left a comment
There was a problem hiding this comment.
For @nickfrosty to review
Problem
Implement the sendTransaction call as a hook
Summary of Changes
as mentioned in #167 to create a sendTransaction hook i have done
useSendTransactioninhooks/send-transaction.tssend-transaction.ts- ensures type safety and correct response shapes for all supported encodingsreactpackageREADME.md