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.

Sign up for GitHub

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

Jump to bottom

Add new command "export_transfers" to save transfers to csv #4236

Merged
merged 1 commit into from
Nov 16, 2018
Merged

Add new command "export_transfers" to save transfers to csv #4236

merged 1 commit into from
Nov 16, 2018

Conversation

sachaaaaa
Copy link
Contributor

This pull request adds a new export_transfers command to the wallet cli that exports the transfers to a csv file.

Since show_transfers and export_transfers basically require the same data, I've added a new function get_transfers that returns a vector of transfer_view structs (new structure as well). That function does all the querying, calculations, sorting and can consume some of the common arguments.
Then, it's up to the caller function to display the data in the appropriate way (print or export as csv).

Notable difference between the csv file and the printed list of transfers:

  • The csv file shows the "running balance" next the the transfer amount. This field is not affected by unconfirmed transfers (pending/failed/pool).
  • To handle transfers with multiple outputs, the csv file displays a new line for any subsequent outputs after the first one, showing only the destination address and amount sent:
x, <total amount>, xxxxx, xxxxxxxxxxxxxxxxx, <address1>, <amount 1>
 ,               ,      ,                  , <address2>, <amount 2>

Changes made to both:

  • display the (shortened) destination address for incoming transfers, to easily match which address the funds were sent to. This will make it easier to match the transfer with the owner's accounts/addresses.

moneromooo-monero
moneromooo-monero reviewed Aug 8, 2018
View reviewed changes
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.

Seems good at first glance, see #4153 though.

src/simplewallet/simplewallet.cpp Outdated
message_writer() << (boost::format("%8.8s %6.6s %25.25s %20.20s %s %s %d %s %s%s") % "pool" % "in" % get_human_readable_timestamp(pd.m_timestamp) % print_money(pd.m_amount) % string_tools::pod_to_hex(pd.m_tx_hash) % payment_id % pd.m_subaddr_index.minor % "-" % note % double_spend_note).str();

transfers.push_back({
tr("pool"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure writing translated data to a formatted file is a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be ok now.
I have used tr() only in show_transfers but not in export_transfers

stoffu
stoffu reviewed Aug 9, 2018
View reviewed changes
src/simplewallet/simplewallet.cpp Outdated
pd.m_tx_hash,
payment_id,
0,
{{"-", pd.m_amount}},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not do the same as in the "in" case?

      std::string destination = m_wallet->get_subaddress_as_str({m_current_subaddress_account, pd.m_subaddr_index.minor});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@sachaaaaa
Copy link
Contributor Author

Thanks guys for the review.
I will update the diff once #4153 is merged.

@moneromooo-monero
Copy link
Collaborator

It is merged now.

@moneromooo-monero
Copy link
Collaborator

ping :)

@moneromooo-monero
Copy link
Collaborator

If you want it in 0.13, now would be a good time to be just in time.

@sachaaaaa
Copy link
Contributor Author

sachaaaaa commented Oct 1, 2018 •
edited
Loading

Hi @moneromooo-monero, thanks for reminding me about this PR.
Let's put this in another time, as I don't know when I'll have time to make the suggested changes.
Thanks!

@sachaaaaa sachaaaaa closed this Oct 1, 2018
@sachaaaaa sachaaaaa reopened this Oct 1, 2018
@moneromooo-monero
Copy link
Collaborator

OK, I'll ping from time to time then.

@moneromooo-monero
Copy link
Collaborator

ping :)

@sachaaaaa
Copy link
Contributor Author

Pong :)

Rebased on master, and all reviews addressed!
From the original diff, I have also added the "unlocked/locked" field in the transfer_view structure.
I was only able to do quick tests with "in", "out" and "block" transfers.

moneromooo-monero
moneromooo-monero reviewed Nov 12, 2018
View reviewed changes
src/simplewallet/simplewallet.cpp Outdated
@@ -7034,7 +7034,7 @@ bool simple_wallet::show_transfers(const std::vector<std::string> &args_)

message_writer(color, false) << formatter
% transfer.block
% transfer.direction
% tr(transfer.direction.c_str())
Copy link
Collaborator

Choose a reason for hiding this comment

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

You use tr on literals, since a tool gathers the literal to put it in a file. So tr should be moved to whatever code assigns transfer.direction.

Copy link
Contributor Author

@sachaaaaa sachaaaaa Nov 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Oh ok, didn't know.
If I move the tr to whatever assigns transfer.direction, then the csv will also be using the translated string. Should I just not use tr at all?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, good point. Hmm. I suppose if they both have to have the same strings, translated is probably best...

@stoffu
Copy link
Contributor

stoffu commented Nov 15, 2018

You can simply drop the last two commits.

fluffypony
fluffypony approved these changes Nov 16, 2018
View reviewed changes
Copy link
Contributor

@fluffypony fluffypony left a comment

Choose a reason for hiding this comment

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

Reviewed

@fluffypony fluffypony merged commit a935714 into monero-project:master Nov 16, 2018
fluffypony added a commit that referenced this pull request Nov 16, 2018
@fluffypony
Merge pull request #4236
1910aab 
a935714 Add new command "export_transfers" to save transfers to csv (sachaaaaa)
@stoffu stoffu mentioned this pull request Jun 18, 2019
Merged
SamsungGalaxyPlayer pushed a commit to SamsungGalaxyPlayer/md that referenced this pull request Jun 9, 2022
Add export_transfers command
65e836f 
monero-project/monero#4236
monero-project/monero#8180
@SamsungGalaxyPlayer SamsungGalaxyPlayer mentioned this pull request Jun 9, 2022
Merged
@kevcrumb kevcrumb mentioned this pull request Jun 11, 2024
Open
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stoffu stoffu stoffu approved these changes

@moneromooo-monero moneromooo-monero moneromooo-monero approved these changes

@fluffypony fluffypony fluffypony approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sachaaaaa @moneromooo-monero @stoffu @fluffypony