Rahul/ifl 2746 wallet sign command #5111
Merged
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
patnir
force-pushed
the
rahul/ifl-2746-generic-walletsign-command
branch
from
6a76a47 to
8598516
Compare
patnir
changed the title
hughy
reviewed
Jul 8, 2024
patnir
force-pushed
the
rahul/ifl-2746-generic-walletsign-command
branch
2 times, most recently
from
86ddb1d to
22e3a96
Compare
hughy
reviewed
Jul 8, 2024
Comment on lines
+53
to
+57
| if (!account.spendingKey) { | ||
| throw new Error('Account does not have a spending key') | ||
| } |
There was a problem hiding this comment.
nit - we could include this check in the predicate function for findAccount, above
There was a problem hiding this comment.
I think distinct messages are better. If the user has the account but it is a view only account, that would be a different error than just not having the account
Comment on lines
46
to
42
| if (!account) { | ||
| const response = await client.wallet.getDefaultAccount() | ||
|
|
||
| if (!response.content.account) { | ||
| this.error( | ||
| `No account is currently active. | ||
| Use ironfish wallet:create <name> to first create an account`, | ||
| ) | ||
| } | ||
|
|
||
| account = response.content.account.name | ||
| } |
There was a problem hiding this comment.
account isn't used now that we can infer it from the transaction
Comment on lines
35
to
33
| watch: Flags.boolean({ | ||
| default: false, | ||
| description: 'Wait for the transaction to be confirmed', | ||
| }), |
There was a problem hiding this comment.
Should this only work if broadcast is set?
patnir
force-pushed
the
rahul/ifl-2746-generic-walletsign-command
branch
from
22e3a96 to
8868138
Compare
patnir
force-pushed
the
rahul/ifl-2746-generic-walletsign-command
branch
from
8868138 to
7c54423
Compare
hughy
approved these changes
Jul 9, 2024
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.
Summary
Adds the
wallet:signCLI command along with thewallet/signTransactionRPC.We already have a
signfunction on an UnsignedTransaction on the Rust side. This basically exposes that feature.Also included is the ledger signing option for this CLI command.
Testing Plan
Manually tested signing an unsigned transaction and adding (wallet:transaction:add) the signed transaction using both an account with a spending key and an account that is a ledger device.
Documentation
To follow in a separate PR on the website
Does this change require any updates to the Iron Fish Docs (ex. the RPC API
Reference)? If yes, link a
related documentation pull request for the website.
Breaking Change
Is this a breaking change? If yes, add notes below on why this is breaking and label it with
breaking-change-rpcorbreaking-change-sdk.