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.

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

fix: set default fees on transfer screen so send all properly works #930

Merged
merged 2 commits into from Jan 4, 2019

Conversation

Projects
None yet
3 participants
@ItsANameToo
Copy link
Collaborator

ItsANameToo commented Jan 4, 2019

Proposed changes

Reported to me in slack by tk0n: the send-all button didn't take into account the 0.1 fee on v1 networks. After looking into it, this seems to be caused by the fees being set on submit, instead of when the form is created. So while using the form, it defaults to a fee of 0 if you haven't touched the inputFee component (which is not available on v1) and will incorrectly calculated the amount you can send. This also seems to be an issue on v2 networks if you don't interact with the inputFee component as it will also default to 0 in that case.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes

ItsANameToo added some commits Jan 4, 2019

@alexbarnsley

This comment has been minimized.

Copy link
Member

alexbarnsley commented Jan 4, 2019

Please can we have some tests 😁

@ItsANameToo

This comment has been minimized.

Copy link
Collaborator

ItsANameToo commented Jan 4, 2019

We'd need an e2e test for that tho, since it's a change in the component usage and not in a function. We don't really seem to have those yet 🤔

@alexbarnsley

This comment has been minimized.

Copy link
Member

alexbarnsley commented Jan 4, 2019

why you raining on my parade? :colbert: @faustbrian bot, halp

@faustbrian

This comment has been minimized.

Copy link
Contributor

faustbrian commented Jan 4, 2019

Then it is time to start adding the first e2e test for that.

@ItsANameToo

This comment has been minimized.

Copy link
Collaborator

ItsANameToo commented Jan 4, 2019

nooo, ok fine

@ItsANameToo

This comment has been minimized.

Copy link
Collaborator

ItsANameToo commented Jan 4, 2019

So looking at this, an e2e test will not be the best approach since it will require a whole workflow to get to the proper page; meaning that half the wallet will be tested just to get to that transaction screen.

Unit test should work, but the TransactionFormTransfer component requires a lot of other components, store things, other defined functions like bip38worker, etc and will take a lot of time to properly mock everything. For now I'll leave the PR as is, and might look into the test if I have time for that or if someone is willing to help out with it.

@faustbrian
Copy link
Contributor

faustbrian left a comment

@faustbrian faustbrian merged commit abd1533 into develop Jan 4, 2019

1 check passed

ci/circleci: test-node-9 Your tests passed on CircleCI!
Details

@faustbrian faustbrian deleted the fix/send-all-fee branch Jan 4, 2019

PHANTOM-DEV1 added a commit to PhantomChain/desktop-wallet that referenced this pull request Jan 9, 2019

PHANTOM-DEV1 added a commit to PhantomChain/desktop-wallet that referenced this pull request Jan 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment