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.

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

Feature/zap #609

Merged
merged 45 commits into from
Jan 23, 2018
Merged

Feature/zap #609

merged 45 commits into from
Jan 23, 2018

Conversation

pciavald
Copy link
Contributor

@pciavald pciavald commented Jan 8, 2018

Ability to ZAP the remaining balance not in cold staking yet.

@pciavald pciavald added the GFX label Jan 8, 2018
@pciavald pciavald self-assigned this Jan 8, 2018
@pciavald pciavald added the WIP label Jan 8, 2018
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 46.03% when pulling d4ebf51 on pciavald:feature/zap into 7816aa9 on particl:dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 46.03% when pulling b35ec33 on pciavald:feature/zap into 7816aa9 on particl:dev.

@pciavald
Copy link
Contributor Author

pciavald commented Jan 8, 2018

@gerlofvanek @AllienWorks what needs to be done here :

  • style zap button (I was thinking of a lightning logo)
  • dialog to tell more about the zap functionality and inform the user of transaction cost, confirm or cancel

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 45.93% when pulling fcd6f5b on pciavald:feature/zap into 7816aa9 on particl:dev.

@pciavald pciavald mentioned this pull request Jan 8, 2018
@pciavald
Copy link
Contributor Author

pciavald commented Jan 8, 2018

  • listunspent before zap to avoid double zap
  • revert cold staking
  • what happens when I spend after cold staking ? staking may be completely stopped for the maturation time

@AllienWorks
Copy link
Member

@anandsinghparihar @vikas-cis can any of you please implement these changes please?

  • "zap" button in Cold Staking widget will now open small modal
  • inside this modal will be the new "zap" button, which will have the "zapping" functionality (as currently has the one in the widget)
  • add the "cancel" button to the modal as well (which will close the modal)

I'll add the rest of the stuff then, style it etc.

Btw @pciavald good thinking about TX cost. We don't have that yet, in the GUI, at all! This needs to be done as well.

@anandsinghparihar
Copy link
Contributor

@AllienWorks I am on it

@anandsinghparihar
Copy link
Contributor

anandsinghparihar commented Jan 9, 2018

@AllienWorks please check
let me know if you want any changes?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 46.026% when pulling b222719 on pciavald:feature/zap into 7816aa9 on particl:dev.

this.log.d('zap sendtypeto simulate', info);

// TODO: ask user to confirm info.fee in a modal
this.dialog.open(ZapWalletsettingsComponent);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anandsinghparihar can you pass the info.fee to the dialog here please ?


private log: any = Log.create('zap-walletsettings');

fee: number = 42;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in order to assign it here

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 46.012% when pulling 58ad326 on pciavald:feature/zap into 7816aa9 on particl:dev.

@anandsinghparihar
Copy link
Contributor

@pciavald request changes pushed !!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 45.997% when pulling 6c5edd2 on pciavald:feature/zap into 7816aa9 on particl:dev.

@anandsinghparihar
Copy link
Contributor

@pciavald I am fixing ng-test and ng-lint

@AllienWorks
Copy link
Member

I've tried this on mainnet, wanted to zap the few remaining %.. Zapped two times, two TX went away, but the progress haven't moved at all.

@pciavald
Copy link
Contributor Author

@AllienWorks zapped two times ? shouldn't be possible :/

@AllienWorks
Copy link
Member

@pciavald I've zapped once first, a balance transfer TX appeared, but the progress % stayed the same, so I waited a few blocks and when still nothing happened, I've zapped for the second time. I was running this branch via npm run start:electron:dev (without the -testnet), not sure if that's the right way to run this on mainnet.. I did that 14/1 22h and left the wallet online unlocked for staking since, hoping it will come through. About another 3% went to cold staking since, but I still have a few % remaining.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.1%) to 45.658% when pulling efd9f89 on pciavald:feature/zap into 3550cd9 on particl:dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.1%) to 45.658% when pulling 215a90a on pciavald:feature/zap into 23d21df on particl:dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 45.609% when pulling 7f91846 on pciavald:feature/zap into 23d21df on particl:dev.

@AllienWorks
Copy link
Member

AllienWorks commented Jan 15, 2018

@pciavald after our session, my testnet wallet (which is not even encrypted) doesn't display the CS widget:

I know it's not a standard behaviour (having your wallet unencrypted), but this should be solved anyway – some of the users might be affected by this (at least the newcomers indeed). I propose having the same style of the widget as "unlock to check state" but with a message like "encrypt wallet to setup cold staking" or something like that (with button pointing to wallet encryption modal ofc).

Edit: if you insert the logic, I can style/write it myself, np.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 46.05% when pulling df034c0 on pciavald:feature/zap into 23d21df on particl:dev.

@anandsinghparihar
Copy link
Contributor

@pciavald test cases is fixed

@anandsinghparihar anandsinghparihar added this to the 1.1 milestone Jan 16, 2018
@pciavald
Copy link
Contributor Author

this still has a bug, @AllienWorks reported wrong zapping logic when zapping on several utxos. We need to investigate this.

@pciavald pciavald mentioned this pull request Jan 16, 2018
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 45.905% when pulling 776e715 on pciavald:feature/zap into fe78910 on particl:dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 45.88% when pulling 166627b on pciavald:feature/zap into fe78910 on particl:dev.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3035

  • 57 of 149 (38.26%) changed or added relevant lines in 5 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.8%) to 45.919%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/app/wallet/wallet/shared/transaction.model.ts 0 1 0.0%
src/app/wallet/wallet/send/send-confirmation-modal/send-confirmation-modal.component.ts 1 2 50.0%
src/app/wallet/overview/widgets/coldstake/zap-coldstaking/zap-coldstaking.component.ts 17 44 38.64%
src/app/wallet/overview/widgets/coldstake/revert-coldstaking/revert-coldstaking.component.ts 19 49 38.78%
src/app/wallet/overview/widgets/coldstake/coldstake.component.ts 20 53 37.74%
Files with Coverage Reduction New Missed Lines %
src/app/wallet/overview/widgets/coldstake/coldstake.component.ts 1 47.87%
Totals Coverage Status
Change from base Build 3034: -0.8%
Covered Lines: 1639
Relevant Lines: 2964

💛 - Coveralls

@pciavald pciavald merged commit 48944e0 into particl:dev Jan 23, 2018
@pciavald pciavald deleted the feature/zap branch January 23, 2018 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants