Fix for importaddress returning incorrect amounts #479

digitaloten · 2019-05-10T09:00:11Z

Fix for importaddress returning incorrect amounts

This is for Issue #193

aguycalled · 2019-05-10T09:10:30Z

The PR does not exactly address the issue. Maybe the description is not clear enough, but I think he refers to an issue when importing cold staking addresses.

digitaloten · 2019-05-10T09:30:12Z

@aguycalled Updated the test case to include cold staking addresses.

aguycalled · 2019-05-12T13:34:14Z

just tested it, importing the spending or staking address in the second node does not show the balance as if it was an address owned by the wallet.

digitaloten · 2019-05-12T16:18:34Z

@aguycalled oh, i assumed that the balance from addresses imported were not to be shown as if it was owned by the wallet, but only show transactions on the watched address. In this case, should commands like getbalance be updated to include balances from watched addresses?

aguycalled · 2019-05-12T18:15:58Z

you are right, only the transactions are watched, but those do not appear for spending or staking addresses.

mxaddict · 2019-05-14T14:44:16Z

you are right, only the transactions are watched, but those do not appear for spending or staking addresses.

So does that mean:

ZZZ = normal address # Has balance
XXX = staking # No balance
YYY = spending # No balance
[YYY,XXX] = cold address # Has balance

Import ZZZ = works (You see transactions + correct amount)
Import XXX = (does this work?)
Import YYY = (does this work?)
Import [YYY,XXX] = works (You see transactions + correct amount)

aguycalled · 2019-05-14T14:46:30Z

you are right, only the transactions are watched, but those do not appear for spending or staking addresses.

So does that mean:

ZZZ = normal address # Has balance
XXX = staking # No balance
YYY = spending # No balance
[YYY,XXX] = cold address # Has balance

Import ZZZ = works (You see transactions + correct amount)
Import XXX = (does this work?)
Import YYY = (does this work?)
Import [YYY,XXX] = works (You see transactions + correct amount)

yes

mxaddict · 2019-05-14T14:55:40Z

@francisjyap I think you just need to add those scenarios to the functional test, then fix the issue 👍 if it still exists.

@aguycalled so the balance is not supposed to update, only the transactions list correct?

aguycalled · 2019-05-14T14:57:16Z

watchonly addresses balance is only shown in qt iirc

… patch-3

…oin#479

digitaloten · 2019-05-15T11:45:15Z

@mxaddict @aguycalled I updated the test cases per your suggestions and the transactions and balance from the imported addresses are reflected correctly.

I added a Watched label on the UI showing the total balance from watched addresses. The Total label is also updated to reflect the total balance plus the total watched balance.

mxaddict · 2019-05-15T12:27:18Z

utACK

…

On Wed, May 15, 2019, 19:45 Francis Yap ***@***.***> wrote: @mxaddict <https://github.com/mxaddict> @aguycalled <https://github.com/aguycalled> I updated the test cases per your suggestions and the transactions and balance from the imported addresses are reflected correctly. I added a Watched label on the UI showing the total balance from watched addresses. The Total label is also updated to reflect the total balance plus the total watched balance. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#479?email_source=notifications&email_token=AAIDAKJWLCCBY4BDLZY7423PVPZU3A5CNFSM4HMBF7Y2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVOMXMQ#issuecomment-492620722>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIDAKKSVHRJHSYMDMT7LM3PVPZU3ANCNFSM4HMBF7YQ> .

mxaddict

Tested on Ubuntu 18.04

Seems legit 👍

But I would like some changes

mxaddict · 2019-05-15T13:55:51Z

qa/rpc-tests/importaddress.py

+
+#import time
+
+class GetStakeReport(NavCoinTestFramework):


This should match the name of the test

mxaddict · 2019-05-15T13:56:05Z

qa/rpc-tests/importaddress.py

+
+
+if __name__ == '__main__':
+    GetStakeReport().main()


Needs to match the name of the file

aguycalled · 2019-05-15T14:15:30Z

qa/rpc-tests/importaddress.py

+        self.nodes[2].importaddress(address)
+        self.nodes[2].importaddress(spending_address)
+        self.nodes[2].importaddress(staking_address)
+        self.nodes[2].importaddress(coldstaking_address)


if you import both the spending/staking and the cold staking address, you can't discern if the txs are seen because the wallet is watching the cold staking address (which was already working) or because it is watching the spending/staking addresses (which were not working before). I'd suggest a setup with one node for each case.

mxaddict · 2019-05-17T15:35:46Z

@francisjyap any updates on this issue?

… cold staking transactions

digitaloten · 2019-05-21T05:30:15Z

@aguycalled @mxaddict So i found out what was causing the cold staking transactions from not appearing when importing a staking/spending address. Cold staking txs would only compare against addresses that are owned by the wallet and not against watched addresses. I also updated the test case.

mxaddict · 2019-05-21T06:13:49Z

utACK

aguycalled · 2019-05-21T12:38:16Z

utACK

mxaddict

Tested and working on Ubuntu 18.04

mxaddict · 2019-05-21T20:33:49Z

Full test suite + new test cases for this scenario also pass

aguycalled · 2019-05-21T21:14:22Z

Tested OSX

aguycalled · 2019-05-22T21:32:51Z

paid 5ab73dece539ab5a29b196a4af43e1834c9a5e35da1c977b92d71a005fb4feed

Fix for importaddress returning incorrect amounts

eda5758

aguycalled assigned digitaloten May 10, 2019

aguycalled added the bounty-med label May 10, 2019

Updated tests case to test cold staking address import as well

4951b69

digitaloten added 4 commits May 15, 2019 10:32

Merge branch 'master' of https://github.com/NAVCoin/navcoin-core into…

03cab62

… patch-3

Updated UI and functional tests to match discussed changes on PR navc…

c972328

…oin#479

Updated var names

0f66d55

Updated test case

4016b71

mxaddict requested review from mxaddict, aguycalled and proletesseract May 15, 2019 13:31

mxaddict suggested changes May 15, 2019

View reviewed changes

aguycalled requested changes May 15, 2019

View reviewed changes

mxaddict added the work in progress label May 18, 2019

digitaloten added 2 commits May 21, 2019 10:41

Updated test case to test each address on different nodes

0755fc6

Added check for spending/staking address against watched addresses on…

8f9a117

… cold staking transactions

mxaddict approved these changes May 21, 2019

View reviewed changes

aguycalled approved these changes May 21, 2019

View reviewed changes

aguycalled added the bounty pending payment label May 21, 2019

aguycalled merged commit b115c2d into navcoin:master May 21, 2019

proletesseract removed the work in progress label May 22, 2019

aguycalled added bounty paid and removed bounty pending payment labels May 22, 2019


		#import time

		class GetStakeReport(NavCoinTestFramework):

Fix for importaddress returning incorrect amounts #479

Fix for importaddress returning incorrect amounts #479

Uh oh!

Conversation

digitaloten commented May 10, 2019

Uh oh!

aguycalled commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

digitaloten commented May 10, 2019

Uh oh!

aguycalled commented May 12, 2019

Uh oh!

digitaloten commented May 12, 2019

Uh oh!

aguycalled commented May 12, 2019

Uh oh!

mxaddict commented May 14, 2019

Uh oh!

aguycalled commented May 14, 2019

Uh oh!

mxaddict commented May 14, 2019

Uh oh!

aguycalled commented May 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

digitaloten commented May 15, 2019

Uh oh!

mxaddict commented May 15, 2019 via email

Uh oh!

mxaddict left a comment

Choose a reason for hiding this comment

Uh oh!

mxaddict May 15, 2019

Choose a reason for hiding this comment

Uh oh!

mxaddict May 15, 2019

Choose a reason for hiding this comment

Uh oh!

aguycalled May 15, 2019

Choose a reason for hiding this comment

Uh oh!

mxaddict commented May 17, 2019

Uh oh!

digitaloten commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mxaddict commented May 21, 2019 via email

Uh oh!

aguycalled commented May 21, 2019

Uh oh!

mxaddict left a comment

Choose a reason for hiding this comment

Uh oh!

mxaddict commented May 21, 2019

Uh oh!

aguycalled commented May 21, 2019

Uh oh!

aguycalled commented May 22, 2019

Uh oh!

Uh oh!

aguycalled commented May 10, 2019 •

edited

Loading

aguycalled commented May 14, 2019 •

edited

Loading

digitaloten commented May 21, 2019 •

edited

Loading