adding ability to select notes in the transaction send command #4465
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
NullSoldier
reviewed
Nov 29, 2023
| @@ -90,6 +90,11 @@ export class Send extends IronfishCommand { | |||
| default: false, | |||
| description: 'Allow offline transaction creation', | |||
| }), | |||
| notes: Flags.string({ | |||
There was a problem hiding this comment.
If you are using the multiple flag, this should be note
This is what it is now
ironfish wallet:send \
--notes 72e91d6672d79f9006f128a16cf52bbb585037fee2aea8385b56919c3ac9570b \
--notes 4a5feb193d425fa73abfb108027a643eb027e46684c50e2bef5d66ea48ec3d2d \
--notes f8b91f81caf4489c83734c2b29ad4587c9ba0c1080854a3f6da19f6fad49265e
this is what it should be
ironfish wallet:send \
--note 72e91d6672d79f9006f128a16cf52bbb585037fee2aea8385b56919c3ac9570b \
--note 4a5feb193d425fa73abfb108027a643eb027e46684c50e2bef5d66ea48ec3d2d \
--note f8b91f81caf4489c83734c2b29ad4587c9ba0c1080854a3f6da19f6fad49265e
There was a problem hiding this comment.
I slightly prefer --notes because you can also do
--notes 72e91d6672d79f9006f128a16cf52bbb585037fee2aea8385b56919c3ac9570b 4a5feb193d425fa73abfb108027a643eb027e46684c50e2bef5d66ea48ec3d2d f8b91f81caf4489c83734c2b29ad4587c9ba0c1080854a3f6da19f6fad49265e
I don't feel strongly so I can change it to --note
There was a problem hiding this comment.
I looked at the docs and I think it seems singular naming conventions are standard, so I'll change it to --note
hughy
reviewed
Nov 29, 2023
| @@ -90,6 +90,11 @@ export class Send extends IronfishCommand { | |||
| default: false, | |||
| description: 'Allow offline transaction creation', | |||
| }), | |||
| notes: Flags.string({ | |||
| char: 'n', | |||
| description: 'The notes to include in the transaction', | |||
There was a problem hiding this comment.
nit - maybe something like note hashes to include? just to be a little more explicit about what needs to be passed here
There was a problem hiding this comment.
yeah note hashes would be more explicit
NullSoldier
approved these changes
Nov 29, 2023
hughy
approved these changes
Nov 29, 2023
patnir
deleted the
rahul/ifl-1845-add-n-notes-flag-to-send-transaction
branch
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
Add ability to send notes in the transaction command.
After a lot of back and forth with these PRs (#4464 and #4459), I decided it was best to at least get this out there for our users and work on more UX improvements including warnings for larger transactions in subsequent PRs.
Testing Plan
Manual testing.
Documentation
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
what additional work is required, if any.