LIT-3995 - (wrapped-keys) - Add signTransaction functionality to supported actions for batchGeneratePrivateKeys
#702
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MaximusHaximus
changed the base branch from
master
to
feat/convert-wrapped-keys-lit-actions-to-ts
MaximusHaximus
changed the title
batchGeneratePrivateKeysbatchGeneratePrivateKeys
…ility to `actions` array processed by `batchGenerateEncryptedKeys` LIT action
…tchGeneratePrivateKeys` api
MaximusHaximus
force-pushed
the
LIT-3995-batchGenKeysAddSignTx
branch
from
250aff5 to
2c075d6
Compare
…h batchGeneratePrivateKeys - This doesn't work for Solana yet, because our test requires the keyPair from the generated key to serialize the TX
MaximusHaximus
changed the title
batchGeneratePrivateKeysbatchGeneratePrivateKeys
Base automatically changed from
feat/convert-wrapped-keys-lit-actions-to-ts
to
master
October 23, 2024 18:16
MaximusHaximus
requested review from
DashKash54
and removed request for
joshLong145
DashKash54
requested changes
Oct 25, 2024
There was a problem hiding this comment.
The reason it works for Ethereum tx is because internally in the signTransactionEthereumKey() we add the from address:
We always need to provide the
from address (and feePayer as well in Solana tx). Since we're generating the wallet and signing right after that the user can just provide the params that we can use to craft a tx object inside the LA itself.Note: The user should always provide the gas and anything else that currently requires an RPC call as we should no longer support making RPC calls from within the LA as it causes arbitrary delay due to the blockchain. Therefore, we shouldn't even support tx broadcast or at least not in the
batchGenerate(). Since we're just creating the wallet the nonce will be 0. @MaximusHaximus lemme know if you have any questions
Comment on lines
+153
to
+155
| if (!ethers.utils.isHexString(signedEthTx)) { | ||
| throw new Error(`signedTx isn't hex: ${signedEthTx}`); | ||
| } |
There was a problem hiding this comment.
Can we verify that the signed tx is from the the generated wallet?
| unsignedTransaction | ||
| ? signTransactionEthereumKey({ | ||
| unsignedTransaction, | ||
| broadcast: action.signTransactionParams?.broadcast || false, |
There was a problem hiding this comment.
We shouldn't allow broadcast in the batchGenerate function as it uses RPC which can take arbitrary amounts of time?
| : Promise.resolve(), | ||
| unsignedTransaction | ||
| ? signTransactionSolanaKey({ | ||
| broadcast: action.signTransactionParams?.broadcast || false, |
There was a problem hiding this comment.
We shouldn't allow broadcast?
| network: Extract<Network, 'solana'>; | ||
| signTransactionParams?: { | ||
| unsignedTransaction: SerializedTransaction; | ||
| broadcast?: boolean; |
There was a problem hiding this comment.
Let's not include the broadcast field it should always be false?
There was a problem hiding this comment.
@MaximusHaximus we don't wanna allow fetching nonce, gas, etc or anything from the RPC. It should all be provided by the user either as a serialized tx or as params and we could construct the transaction inside the LA but we should not fetch anything from the RPC we can remove this fetching altogether
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
fromPubkeyandfeePayerin the serialized TX to match the pubKey of the generated key that is doing the signer -- but obviously that can't happen since we generate the key in the LIT action runtime. 🤔Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
local-tests/tests/wrapped-keys/testBatchGeneratePrivateKeys.tsto sign an ethereum tx and verify the signature returned is hex (same logic as our existing signTransactionEthereum test)Checklist: