Skip to content

Send max#5108

Merged
cmgustavo merged 19 commits intobitpay:masterfrom
JDonadio:feat/send-max
Dec 1, 2016
Merged

Send max#5108
cmgustavo merged 19 commits intobitpay:masterfrom
JDonadio:feat/send-max

Conversation

@JDonadio
Copy link
Copy Markdown
Contributor

@JDonadio JDonadio commented Nov 24, 2016 •
edited
Loading

Closes https://github.com/bitpay/copay/issues/4884

  • Add menu to display send max option
  • Display wallet selector in confirm view without default selection
  • Select wallet to get send max information (add to cache list)
  • Fix feeService
  • Fix paperWallet current fee value

@gabrielbazan7
Copy link
Copy Markdown
Collaborator

gabrielbazan7 commented Nov 24, 2016

It's recommended to use $timeout with 10 when calling to $ionicScrollDelegate.resize()

@gabrielbazan7
Copy link
Copy Markdown
Collaborator

gabrielbazan7 commented Nov 24, 2016

What is isWallet variable for ?

gabrielbazan7
gabrielbazan7 reviewed Nov 24, 2016
View reviewed changes
Comment thread src/js/controllers/amount.js Outdated
Copy link
Copy Markdown
Collaborator

@gabrielbazan7 gabrielbazan7 Nov 24, 2016

Choose a reason for hiding this comment

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

This both variable are unused

Copy link
Copy Markdown
Contributor Author

@JDonadio JDonadio Nov 25, 2016

Choose a reason for hiding this comment

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

You're right, thanks!

@JDonadio
Copy link
Copy Markdown
Contributor Author

JDonadio commented Nov 25, 2016

What is isWallet variable for ?

I think it's used to show/hide contact card/address box style

@JDonadio JDonadio force-pushed the feat/send-max branch 5 times, most recently from 587b364 to 8cbab31 Compare November 25, 2016 14:51
@cmgustavo
Copy link
Copy Markdown
Member

cmgustavo commented Nov 28, 2016

Still getting Insufficient funds for fee

@cmgustavo
Copy link
Copy Markdown
Member

cmgustavo commented Nov 28, 2016

Second time, I choose Send max amount, I can see all my wallets (also with 0 bits). I shouldn't see wallets without funds.

bitpay_-_secure_bitcoin_wallet

@JDonadio
Copy link
Copy Markdown
Contributor Author

JDonadio commented Nov 29, 2016

Updated

@JDonadio JDonadio self-assigned this Nov 29, 2016
isocolsky
isocolsky reviewed Nov 29, 2016
View reviewed changes
Comment thread src/js/controllers/confirm.js Outdated
Copy link
Copy Markdown
Contributor

@isocolsky isocolsky Nov 29, 2016

Choose a reason for hiding this comment

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

Why assume an error means insufficient funds?

Copy link
Copy Markdown
Contributor Author

@JDonadio JDonadio Nov 29, 2016

Choose a reason for hiding this comment

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

Shouldn't be there. Done

Comment thread src/js/controllers/confirm.js Outdated
Copy link
Copy Markdown
Contributor

@isocolsky isocolsky Nov 29, 2016

Choose a reason for hiding this comment

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

Add a '.' at the end.

Copy link
Copy Markdown
Contributor Author

@JDonadio JDonadio Nov 29, 2016

Choose a reason for hiding this comment

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

Done

Comment thread src/js/controllers/confirm.js Outdated
Copy link
Copy Markdown
Contributor

@isocolsky isocolsky Nov 29, 2016

Choose a reason for hiding this comment

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

Remove '.' at the beginning.

Copy link
Copy Markdown
Contributor Author

@JDonadio JDonadio Nov 29, 2016

Choose a reason for hiding this comment

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

Done

Comment thread src/js/controllers/confirm.js Outdated
Copy link
Copy Markdown
Contributor

@isocolsky isocolsky Nov 29, 2016

Choose a reason for hiding this comment

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

Add '.' at the end.

Copy link
Copy Markdown
Contributor Author

@JDonadio JDonadio Nov 29, 2016

Choose a reason for hiding this comment

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

Done

Comment thread src/js/controllers/confirm.js Outdated
Copy link
Copy Markdown
Contributor

@isocolsky isocolsky Nov 29, 2016

Choose a reason for hiding this comment

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

Why not use setWallet(wallet)?

Copy link
Copy Markdown
Contributor Author

@JDonadio JDonadio Nov 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

setWallet() method trigger createTx without alert message with send max info.

Copy link
Copy Markdown
Contributor Author

@JDonadio JDonadio Dec 1, 2016

Choose a reason for hiding this comment

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

Also is not getting the send max info before build the tx

Comment thread src/js/controllers/confirm.js Outdated
Copy link
Copy Markdown
Contributor

@isocolsky isocolsky Nov 29, 2016

Choose a reason for hiding this comment

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

Watch out for rounding errors.

Copy link
Copy Markdown
Contributor Author

@JDonadio JDonadio Nov 29, 2016

Choose a reason for hiding this comment

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

Great catch. Done

Comment thread src/js/services/feeService.js Outdated
Copy link
Copy Markdown
Contributor

@isocolsky isocolsky Nov 29, 2016

Choose a reason for hiding this comment

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

It is better to include the argument inside the string so translators can do a better job.

Copy link
Copy Markdown
Contributor Author

@JDonadio JDonadio Nov 29, 2016

Choose a reason for hiding this comment

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

Done

cmgustavo
cmgustavo reviewed Dec 1, 2016
View reviewed changes
Comment thread src/js/controllers/preferencesFee.js Outdated
Copy link
Copy Markdown
Member

@cmgustavo cmgustavo Dec 1, 2016

Choose a reason for hiding this comment

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

getFeeLevels function doesn't return err. Please, check the file feeServices.js

Copy link
Copy Markdown
Member

@cmgustavo cmgustavo Dec 1, 2016

Choose a reason for hiding this comment

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

Ignore. I didn't see the file updated.

@JDonadio JDonadio changed the title WIP send max Send max Dec 1, 2016
@cmgustavo
Copy link
Copy Markdown
Member

cmgustavo commented Dec 1, 2016

ACK! Great job!

@cmgustavo cmgustavo merged commit 1efad56 into bitpay:master Dec 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cmgustavo cmgustavo cmgustavo left review comments

@gabrielbazan7 gabrielbazan7 gabrielbazan7 left review comments

+1 more reviewer

@isocolsky isocolsky isocolsky left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@JDonadio JDonadio

Labels

In Progress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@JDonadio @gabrielbazan7 @cmgustavo @isocolsky