Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add ability to compare selection strategies #2516

Merged
merged 1 commit into from
Feb 12, 2019
Merged

Add ability to compare selection strategies #2516

merged 1 commit into from
Feb 12, 2019

Conversation

i1skn
Copy link
Contributor

@i1skn i1skn commented Feb 1, 2019

Before tx creation user can estimate fee and locked amount
with different selection strategies by providing -e flag for
wallet send command.

@i1skn
Add ability to compare selection strategies
7729859 
Before tx creation user can estimate fee and locked amount
with different selection strategies by providing `-e` flag for
`wallet send` command.
hashmap
hashmap reviewed Feb 2, 2019
View reviewed changes
Copy link
Contributor

@hashmap hashmap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you consider implementing estimate as a separate command? From UX point of view if a user wants to estimate fee she doesn't need to provide any additional send parameters like destination. It would also make the logic simpler.

src/bin/cmd/wallet_args.rs
command::send(
inst_wallet(),
a,
wallet_config.dark_background_color_scheme.unwrap_or(true),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it unrelated change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed, because send command would able to emit a table with information, which uses colors.

@i1skn
Copy link
Contributor Author

i1skn commented Feb 3, 2019

Would you consider implementing estimate as a separate command? From UX point of view if a user wants to estimate fee she doesn't need to provide any additional send parameters like destination. It would also make the logic simpler.

I've got my inspiration from git clean command, where you can just provide --dry-run to see, what will be deleted before actually do it. So the user can see, what will happen after sending without doing it. Also, if the user provides -e flag then destination becomes not required. So grin wallet send -e 123 would show the estimation, but grin wallet send 123 would still prompt for destination.

@0xmichalis
Copy link
Contributor

Why not use --dry-run which is clearer than -e?

@i1skn
Copy link
Contributor Author

i1skn commented Feb 4, 2019

@Kargakis short version -d is already taken for a --destination. -e states for --estimate-selection which, I think, also makes sense.

@hashmap hashmap requested a review from yeastplume February 5, 2019 10:19
@ignopeverell ignopeverell added this to the 1.0.2 milestone Feb 6, 2019
@yeastplume
Copy link
Member

Looks good for now. Might want to explore refactoring some of the underlying coin selection calls to eliminate a bit of redundancy, but can leave that until after the wallet is split off.

@yeastplume yeastplume merged commit 1d0c04c into mimblewimble:master Feb 12, 2019
yeastplume pushed a commit that referenced this pull request Feb 13, 2019
@svechinsky @yeastplume
Feature/slate serialization (#2534)
ee4eed7 
* - Add backwards compatability
- Add hex serialization

* rustfmt

* rustfmt

* Windows Compatibility Fixes #1 (#2535)

* initial changes for windows build and unit/integration tests

* rustfmt

* wallet+store tests

* rustfmt

* fix linux daemonize

* better encapsulate file rename

* rustfmt

* remove daemonize commands

* rustfmt

* remove server start/stop commands

* add ability to drop pmmr backend files explicitly for txhashset unzip

* rustfmt

* fix pmmr tests

* rustfmt

* Windows TUI Fix (#2555)

* switch pancurses backend to win32

* revert changes to restore test

* compatibility fix + debug messages

* rustfmt

* Add content disposition for OK responses  (#2545)

* Testing http send and fixing accordingly

* add repost method into wallet owner api (#2553)

* add repost method into wallet owner api

* rustfmt

* Add ability to compare selection strategies (#2516)

Before tx creation user can estimate fee and locked amount
with different selection strategies by providing `-e` flag for
`wallet send` command.
@hashmap hashmap deleted the estimate-send branch April 4, 2019 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hashmap hashmap hashmap left review comments

@yeastplume yeastplume Awaiting requested review from yeastplume

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
1.0.2
Development

Successfully merging this pull request may close these issues.
5 participants
@i1skn @0xmichalis @yeastplume @hashmap @ignopeverell