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

7344 Fix describe_transfer in the case of multiple transactions in the set #7349

Conversation

lxop
Copy link

@lxop lxop commented Jan 25, 2021

  • Reset the recipients map for each transaction so that the output only shows the recipients for each individual transaction in the set.
  • Add a field to the output of describe_transfer that includes the total amounts and the aggregated set of recipients.

Fixes #7344

@lxop lxop changed the title 7344 describe transfer fix multiple tx case 7344 Fix describe_transfer in the case of multiple transactions in the set Jan 25, 2021
Copy link
Collaborator

@moneromooo-monero moneromooo-monero left a comment

Choose a reason for hiding this comment

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

The dests renaming is a bit spammy but small enough I guess.

}

if (desc.change_amount > 0)
{
const tools::wallet2::tx_construction_data &cd0 = tx_constructions[0];
desc.change_address = get_account_address_as_str(m_wallet->nettype(), cd0.subaddr_account > 0, cd0.change_dts.addr);
res.summary.change_address = desc.change_address;
Copy link
Collaborator

Choose a reason for hiding this comment

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

That address, if any, could be different for all txes.

Copy link
Author

Choose a reason for hiding this comment

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

If that were the case an error would be triggered on line 1278, so it is safe to assume all change goes to one address here.

@lxop
Copy link
Author

lxop commented Jan 31, 2021

The dests renaming is a bit spammy but small enough I guess.

@moneromooo-monero I renamed dests -> tx_dests because there are now two "destinations" maps and I wanted more clarity about the purpose of each of them and which one is being used on any given line.

@selsta
Copy link
Collaborator

selsta commented Feb 8, 2021

Can you bump WALLET_RPC_VERSION_MINOR ?

@selsta
Copy link
Collaborator

selsta commented Feb 8, 2021

Thanks, please squash your commits and then we can add this to the merge list.

This ensures each list of recipients is only the recipients
for one transaction. It also adds a new field "summary"
that describes the txset as a whole.

Fixes monero-project#7344
@lxop lxop force-pushed the 7344-describe_transfer-fix-multiple-tx-case branch from e505449 to bf751f4 Compare February 8, 2021 20:34
@selsta
Copy link
Collaborator

selsta commented Mar 26, 2021

@lxop could you please rebase? src/wallet/wallet_rpc_server.cpp conflicts due to a recent merge

@selsta
Copy link
Collaborator

selsta commented Jun 21, 2021

ping @lxop, I would appreciate it if you could rebase so that we can merge this.

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.

describe_transfer has incorrect output when multiple transactions are present
4 participants