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][Zerocoin] transaction list - handle receive from zerocoin transactions #946

Merged
merged 1 commit into from
May 18, 2021

Conversation

codeofalltrades
Copy link
Collaborator

Problem

The wallet transaction list was updated to show the correct address #898
Zerocoins sent to a bv1 address were causing the force_return error. The address format is bech32 however the decode method was trying to decode the address as bech32 = false.

Root Cause

The address format is bech32 however the decode method was trying to decode the address as bech32 = false.

Solution

Decode the address using the correct format.

Unit Testing Results

  1. Create a new wallet
  2. Get a basecoin address
  3. Spend Zerocoins from a different wallet to the basecoin address
  4. Verfiy the tx(s) are confirmed in the new wallet
  5. Restart the wallet(veil-qt) with -reindex-chainstate
  6. Verify the force_return error does not occur

@codeofalltrades codeofalltrades added Component: GUI Primarily related to the display of the user interface Tag: TransactionRecords The display of transaction information Tag: Waiting For Code Review Waiting for code review from a core developer labels May 17, 2021
@codeofalltrades codeofalltrades self-assigned this May 17, 2021
Comment on lines +414 to +417
bool fBech32 = false;
if (boost::get<CScriptID>(&wtx.txout_address[i])){
fBech32 = true;
}
Copy link
Collaborator

@Zannick Zannick May 17, 2021

Choose a reason for hiding this comment

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

NIT: Can this be condensed into a single line instead of the if? say, something like
bool fBech32 = (bool)boost::get<CScriptID>(...); or boost::get<CScriptID>(...) != ... ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's true:
bool fBech32 = boost::get<CScriptID>(&wtx.txout_address[i]) would do the trick; but functionally it's the same and and it's not against style; so I'll say that's a NIT

Another NIT: space between the end of the if and the {

But given problems with this problem, I'm not concerned with holding it up.

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.

utACK 352d78e

Comment on lines +414 to +417
bool fBech32 = false;
if (boost::get<CScriptID>(&wtx.txout_address[i])){
fBech32 = true;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's true:
bool fBech32 = boost::get<CScriptID>(&wtx.txout_address[i]) would do the trick; but functionally it's the same and and it's not against style; so I'll say that's a NIT

Another NIT: space between the end of the if and the {

But given problems with this problem, I'm not concerned with holding it up.

@WetOne
Copy link
Collaborator

WetOne commented May 18, 2021

utACK 352d78e

@CaveSpectre11 CaveSpectre11 added Code Review: Passed and removed Tag: Waiting For Code Review Waiting for code review from a core developer labels May 18, 2021
@CaveSpectre11 CaveSpectre11 merged commit 15a0b61 into Veil-Project:master May 18, 2021
@CaveSpectre11 CaveSpectre11 added the Dev Status: Merged Issue is completely finished. label May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Review: Passed Component: GUI Primarily related to the display of the user interface Dev Status: Merged Issue is completely finished. Tag: TransactionRecords The display of transaction information
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants