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

[GUI] Update QR Code on address selection #888

Merged
merged 1 commit into from
Jan 18, 2021

Conversation

WetOne
Copy link
Collaborator

@WetOne WetOne commented Jan 2, 2021

Problem

Selection of an address on the Address Book page does not update the QR codes on the receive page or the left hand side.

Root Cause

Selection of an address on the Address Book page is isolated to the address book.

Solution

Notify the other displays to allow for updates based on the selection of an address. The update includes displaying address, QR code, name (if any).

Issues Addressed

250: #250
136: #136

@CaveSpectre11 CaveSpectre11 added Bounty: Assigned This issue is being worked on by someone that will earn a bounty reward. Component: GUI Primarily related to the display of the user interface Component: Wallet Relating to keystore, tx creation, and balance tracking Tag: Waiting For Code Review Waiting for code review from a core developer Tag: Waiting For Developer and removed Tag: Waiting For Code Review Waiting for code review from a core developer labels Jan 2, 2021
@CaveSpectre11
Copy link
Collaborator

Investigating oddity with it pulling contact addresses.

@@ -240,6 +243,16 @@ void AddressesWidget::handleAddressClicked(const QModelIndex &index){
listView = ui->listAddresses;
type = AddressTableModel::Receive;
updatedIndex = proxyModel->mapToSource(index);
auto address = this->model->index(index.row(), AddressTableModel::Address, index);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be the updatedIndex.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the line. both index.row() and index (the last argument) have been updated to updatedIndex.

@codeofalltrades
Copy link
Collaborator

In the console enter getnewminingaddress
Go to the contacts tab
Select the mining address.
Displayed address wrong
The copy button copies the incorrect address.
image

@CaveSpectre11 CaveSpectre11 added Tag: Waiting For Code Review Waiting for code review from a core developer and removed Tag: Waiting For Developer labels Jan 3, 2021
@CaveSpectre11
Copy link
Collaborator

In the console enter getnewminingaddress
Go to the contacts tab
Select the mining address.
Displayed address wrong

I think it's decoding it into a destination address:

        LogPrintf("%s: index %d %d, updatedindex %d %d, addressStr = %s\n", __func__,
                  index.row(), index.column(), updatedIndex.row(), updatedIndex.column(), addressStr.toStdString().c_str());
        CTxDestination selectedAddress = DecodeDestination(addressStr.toStdString());
2021-01-03T03:32:52Z handleAddressClicked: index 6 0, updatedindex 8 0, addressStr = VKR4AXDC3mRq2zNfAhrMYgh1i74FbP6XwW

Probably have to dig into the bowels of the CTxDestination to determine if it should leave it as a raw address or decode it.

@codeofalltrades
Copy link
Collaborator

copy via the tooltip copies the address that is displayed.
image
muQ3R2f8o2cf9S4gRGTxPJvXgU7zcP2a8z

@WetOne
Copy link
Collaborator Author

WetOne commented Jan 3, 2021

@codeofalltrades, I think @CaveSpectre11 is correct - it is trying to decode the passed address into a destination address. The code was originally intended to only show a stealth receiving address (specifically, a stealth receiving address named "stealth", if you didn't have one, one is created for you). The implementation was not intended to have anything other than a receiving address displayed. What is your preference: to only have stealth receiving addresses displayed, or be able to have all non-contact addresses displayed (any address the My Addresses tab)?

@codeofalltrades
Copy link
Collaborator

or be able to have all non-contact addresses displayed (any address the My Addresses tab)

From a consistency perspective it should be all addresses. However with the move away from other coin types/addresses doing the work to support those types doesn't make sense resource wise. @CaveSpectre11 thoughts?

@CaveSpectre11
Copy link
Collaborator

or be able to have all non-contact addresses displayed (any address the My Addresses tab)

From a consistency perspective it should be all addresses. However with the move away from other coin types/addresses doing the work to support those types doesn't make sense resource wise. @CaveSpectre11 thoughts?

I'm not sure how much work it would be to recognize the coin type. It probably is possible, I'm sure there's a routine. If there's a routine to check it and it's a couple lines of code (if !miningAddr, Decode destination), then it might be worthwhile since we don't have mining to stealth yet.

But if it's not simple, probably let it go. But test it and see if you can send to that decoded address. That would change the priority. If they can't send to the decoded address, then that's a definite problem and we'll need to detect it.

If we can send to the decoded address; I'll let you make the severity call.

@codeofalltrades
Copy link
Collaborator

If we can send to the decoded address

looks like a no on the decoded address.
image

@CaveSpectre11 CaveSpectre11 added Tag: Waiting For Developer and removed Tag: Waiting For Code Review Waiting for code review from a core developer labels Jan 4, 2021
@CaveSpectre11
Copy link
Collaborator

If we can send to the decoded address

looks like a no on the decoded address.

I'm wondering if it's a problem in decode destination; where the leading character isn't recognized; or otherwise decoding incorrectly for a mining address; which could also be related to the stuck mining spend problem.

@WetOne
Copy link
Collaborator Author

WetOne commented Jan 16, 2021

@CaveSpectre11 , @codeofalltrades : This is almost definitely the encode/decode. Do we know what needs to be changed other than the mining address? The update for the mining address looks simple enough. I would like to make the change once.

@WetOne
Copy link
Collaborator Author

WetOne commented Jan 16, 2021

With a little more digging I found that the problem appears in the lines where EncodeDestination(arg1, arg2) is called. In my implementation arg2 is always set to true. This needs to be changed to be "if !(receive_miner == purpose)" in both balance.cpp and receive address widget.

The change will definitely fix the issue with receiving miner addresses. I don't see any indication in the code that more needs to be done for other addresses. Let me know if you want me to just push the one change or if another update needs to be made.

@codeofalltrades
Copy link
Collaborator

My vote is push the one change.

@CaveSpectre11
Copy link
Collaborator

My vote is push the one change.

Agreed. Let's get it in here and make sure everything looks the way that's needed.

@WetOne
Copy link
Collaborator Author

WetOne commented Jan 17, 2021

@CaveSpectre11, @codeofalltrades
Updated. Testing with mining addresses worked.

@CaveSpectre11 CaveSpectre11 self-requested a review January 17, 2021 20:59
@CaveSpectre11 CaveSpectre11 added Tag: Waiting For Code Review Waiting for code review from a core developer and removed Tag: Waiting For Developer labels Jan 17, 2021
Copy link
Collaborator

@CaveSpectre11 CaveSpectre11 left a comment

Choose a reason for hiding this comment

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

ACK 7cf0778

Copy link
Collaborator

@codeofalltrades codeofalltrades left a comment

Choose a reason for hiding this comment

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

ACK 7cf0778

@codeofalltrades codeofalltrades merged commit 50f1f00 into Veil-Project:master Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bounty: Assigned This issue is being worked on by someone that will earn a bounty reward. Component: GUI Primarily related to the display of the user interface Component: Wallet Relating to keystore, tx creation, and balance tracking Tag: Waiting For Code Review Waiting for code review from a core developer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants