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

Refactor get_payments for future separation from wallet2 #5598

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 28 additions & 5 deletions src/wallet/wallet2.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5683,9 +5683,15 @@ void wallet2::get_payments(const crypto::hash& payment_id, std::list<wallet2::pa
});
}
//----------------------------------------------------------------------------------------------------
void wallet2::get_payments(std::list<std::pair<crypto::hash,wallet2::payment_details>>& payments, uint64_t min_height, uint64_t max_height, const boost::optional<uint32_t>& subaddr_account, const std::set<uint32_t>& subaddr_indices) const
{
auto range = std::make_pair(m_payments.begin(), m_payments.end());
void wallet2::get_payments(
std::list<std::pair<crypto::hash,wallet2::payment_details>>& payments,
const payment_container& actual_payments,
uint64_t min_height,
uint64_t max_height,
const boost::optional<uint32_t>& subaddr_account,
const std::set<uint32_t>& subaddr_indices) const
{
auto range = std::make_pair(actual_payments.begin(), actual_payments.end());
std::for_each(range.first, range.second, [&payments, &min_height, &max_height, &subaddr_account, &subaddr_indices](const payment_container::value_type& x) {
if (min_height < x.second.m_block_height && max_height >= x.second.m_block_height &&
(!subaddr_account || *subaddr_account == x.second.m_subaddr_index.major) &&
Expand All @@ -5696,10 +5702,21 @@ void wallet2::get_payments(std::list<std::pair<crypto::hash,wallet2::payment_det
});
}
//----------------------------------------------------------------------------------------------------
void wallet2::get_payments_out(std::list<std::pair<crypto::hash,wallet2::confirmed_transfer_details>>& confirmed_payments,
void wallet2::get_payments(std::list<std::pair<crypto::hash,wallet2::payment_details>>& payments,
uint64_t min_height, uint64_t max_height, const boost::optional<uint32_t>& subaddr_account, const std::set<uint32_t>& subaddr_indices) const
{
for (auto i = m_confirmed_txs.begin(); i != m_confirmed_txs.end(); ++i) {
get_payments(payments, m_payments, min_height, max_height, subaddr_account, subaddr_indices);
}
//----------------------------------------------------------------------------------------------------
void wallet2::get_payments_out(
std::list<std::pair<crypto::hash,wallet2::confirmed_transfer_details>>& confirmed_payments,
const std::unordered_map<crypto::hash, confirmed_transfer_details>& actual_confirmed_txs,
uint64_t min_height,
uint64_t max_height,
const boost::optional<uint32_t>& subaddr_account,
const std::set<uint32_t>& subaddr_indices) const
{
for (auto i = actual_confirmed_txs.begin(); i != actual_confirmed_txs.end(); ++i) {
if (i->second.m_block_height <= min_height || i->second.m_block_height > max_height)
continue;
if (subaddr_account && *subaddr_account != i->second.m_subaddr_account)
Expand All @@ -5710,6 +5727,12 @@ void wallet2::get_payments_out(std::list<std::pair<crypto::hash,wallet2::confirm
}
}
//----------------------------------------------------------------------------------------------------
void wallet2::get_payments_out(std::list<std::pair<crypto::hash,wallet2::confirmed_transfer_details>>& confirmed_payments,
uint64_t min_height, uint64_t max_height, const boost::optional<uint32_t>& subaddr_account, const std::set<uint32_t>& subaddr_indices) const
{
get_payments_out(confirmed_payments, m_confirmed_txs, min_height, max_height, subaddr_account, subaddr_indices);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you're breaking compatibility anyway, please remove the "min_height_inclusive" suffix.

Copy link
Author

Choose a reason for hiding this comment

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

Done (0c43135).

//----------------------------------------------------------------------------------------------------
void wallet2::get_unconfirmed_payments_out(std::list<std::pair<crypto::hash,wallet2::unconfirmed_transfer_details>>& unconfirmed_payments, const boost::optional<uint32_t>& subaddr_account, const std::set<uint32_t>& subaddr_indices) const
{
for (auto i = m_unconfirmed_txs.begin(); i != m_unconfirmed_txs.end(); ++i) {
Expand Down
3 changes: 3 additions & 0 deletions src/wallet/wallet2.h
Original file line number Diff line number Diff line change
Expand Up @@ -854,7 +854,10 @@ namespace tools
bool check_connection(uint32_t *version = NULL, bool *ssl = NULL, uint32_t timeout = 200000);
void get_transfers(wallet2::transfer_container& incoming_transfers) const;
void get_payments(const crypto::hash& payment_id, std::list<wallet2::payment_details>& payments, uint64_t min_height = 0, const boost::optional<uint32_t>& subaddr_account = boost::none, const std::set<uint32_t>& subaddr_indices = {}) const;
void get_payments(std::list<std::pair<crypto::hash,wallet2::payment_details>>& payments, const payment_container& actual_payments, uint64_t min_height, uint64_t max_height = (uint64_t)-1, const boost::optional<uint32_t>& subaddr_account = boost::none, const std::set<uint32_t>& subaddr_indices = {}) const;
void get_payments(std::list<std::pair<crypto::hash,wallet2::payment_details>>& payments, uint64_t min_height, uint64_t max_height = (uint64_t)-1, const boost::optional<uint32_t>& subaddr_account = boost::none, const std::set<uint32_t>& subaddr_indices = {}) const;
void get_payments_out(std::list<std::pair<crypto::hash,wallet2::confirmed_transfer_details>>& confirmed_payments, const std::unordered_map<crypto::hash, confirmed_transfer_details>& actual_confirmed_txs,
uint64_t min_height, uint64_t max_height = (uint64_t)-1, const boost::optional<uint32_t>& subaddr_account = boost::none, const std::set<uint32_t>& subaddr_indices = {}) const;
void get_payments_out(std::list<std::pair<crypto::hash,wallet2::confirmed_transfer_details>>& confirmed_payments,
uint64_t min_height, uint64_t max_height = (uint64_t)-1, const boost::optional<uint32_t>& subaddr_account = boost::none, const std::set<uint32_t>& subaddr_indices = {}) const;
void get_unconfirmed_payments_out(std::list<std::pair<crypto::hash,wallet2::unconfirmed_transfer_details>>& unconfirmed_payments, const boost::optional<uint32_t>& subaddr_account = boost::none, const std::set<uint32_t>& subaddr_indices = {}) const;
Expand Down
1 change: 1 addition & 0 deletions tests/unit_tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ set(unit_tests_sources
epee_utils.cpp
expect.cpp
fee.cpp
get_payments.cpp
json_serialization.cpp
get_xtype_from_string.cpp
hashchain.cpp
Expand Down
235 changes: 235 additions & 0 deletions tests/unit_tests/get_payments.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,235 @@
// Copyright (c) 2014-2019, The Monero Project
//
// All rights reserved.
//
// Redistribution and use in source and binary forms, with or without modification, are
// permitted provided that the following conditions are met:
//
// 1. Redistributions of source code must retain the above copyright notice, this list of
// conditions and the following disclaimer.
//
// 2. Redistributions in binary form must reproduce the above copyright notice, this list
// of conditions and the following disclaimer in the documentation and/or other
// materials provided with the distribution.
//
// 3. Neither the name of the copyright holder nor the names of its contributors may be
// used to endorse or promote products derived from this software without specific
// prior written permission.
//
// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY
// EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
// MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
// THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
// PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
// INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
// STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
// THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

// FIXME: move this into a full wallet2 unit test suite, if possible

#include "gtest/gtest.h"

#include "wallet/wallet2.h"
#include <string>


class GetPayments : public ::testing::Test
{
protected:
virtual void SetUp()
{
try
{
m_wallet.generate("", password, recovery_key, true, false);
}
catch (const std::exception& e)
{
LOG_ERROR("failed to generate wallet: " << e.what());
throw;
}

payment.m_block_height = 7;
transfer.m_block_height = 7;
block_height_containing_transaction = payment.m_block_height;

actual_payments.insert( {{payment_id, payment}} );
actual_confirmed_txs.insert( {{payment_id, transfer}} );

matched_payments.clear();
confirmed_payments.clear();
ASSERT_EQ(0, matched_payments.size());
ASSERT_EQ(0, confirmed_payments.size());
}

virtual void TearDown()
{
}


tools::wallet2 m_wallet;
const std::string password = "testpass";
crypto::secret_key recovery_key = crypto::secret_key();
crypto::hash payment_id;

uint64_t min_height = 0;
uint64_t max_height = (uint64_t)-1;

uint64_t block_height_containing_transaction;

// for use with get_payments()
tools::wallet2::payment_details payment;
std::list<std::pair<crypto::hash, tools::wallet2::payment_details>> matched_payments;
tools::wallet2::payment_container actual_payments;

// for use with get_payments_out()
tools::wallet2::confirmed_transfer_details transfer;
std::list<std::pair<crypto::hash, tools::wallet2::confirmed_transfer_details>> confirmed_payments;
std::unordered_map<crypto::hash, tools::wallet2::confirmed_transfer_details> actual_confirmed_txs;
};

TEST_F(GetPayments, FindsMultiplePaymentsAtSameHeight)
{
payment.m_block_height = 6;
actual_payments.insert( {{payment_id, payment}} );

// One payment is already added by SetUp(). This adds another at the same height.
payment.m_block_height = block_height_containing_transaction;
actual_payments.insert( {{payment_id, payment}} );

m_wallet.get_payments(matched_payments, actual_payments,
--block_height_containing_transaction, block_height_containing_transaction);
ASSERT_EQ(2, matched_payments.size());
}
TEST_F(GetPayments, FindsMultiplePaymentsAtSameHeight_Out)
{
crypto::hash payment_id2;
crypto::hash payment_id3;
crypto::hash payment_id4;

transfer.m_block_height = 6;
actual_confirmed_txs.insert( {{payment_id2, transfer}} );

transfer.m_block_height = 8;
actual_confirmed_txs.insert( {{payment_id3, transfer}} );

// One payment is already added by SetUp(). This adds another at the same height.
transfer.m_block_height = block_height_containing_transaction;
actual_confirmed_txs.insert( {{payment_id4, transfer}} );

m_wallet.get_payments_out(confirmed_payments, actual_confirmed_txs,
--block_height_containing_transaction, block_height_containing_transaction);
ASSERT_EQ(2, confirmed_payments.size());
}


TEST_F(GetPayments, NotFoundBelowMinHeight)
{
m_wallet.get_payments(matched_payments, actual_payments,
++block_height_containing_transaction, block_height_containing_transaction);
ASSERT_EQ(0, matched_payments.size());
}
TEST_F(GetPayments, NotFoundBelowMinHeight_Out)
{
m_wallet.get_payments_out(confirmed_payments, actual_confirmed_txs,
++block_height_containing_transaction, block_height_containing_transaction);
ASSERT_EQ(0, confirmed_payments.size());
}


TEST_F(GetPayments, NotFoundAboveMaxHeight)
{
m_wallet.get_payments(matched_payments, actual_payments,
block_height_containing_transaction, --block_height_containing_transaction);
ASSERT_EQ(0, matched_payments.size());
}
TEST_F(GetPayments, NotFoundAboveMaxHeight_Out)
{
m_wallet.get_payments_out(confirmed_payments, actual_confirmed_txs,
block_height_containing_transaction, --block_height_containing_transaction);
ASSERT_EQ(0, confirmed_payments.size());
}


TEST_F(GetPayments, NotFoundIfNegative)
{
m_wallet.get_payments(matched_payments, actual_payments,
-1, -9999);
ASSERT_EQ(0, matched_payments.size());
}
TEST_F(GetPayments, NotFoundIfNegative_Out)
{
m_wallet.get_payments_out(confirmed_payments, actual_confirmed_txs,
-1, -9999);
ASSERT_EQ(0, confirmed_payments.size());
}


TEST_F(GetPayments, NotFoundIfNegativeReversed)
{
m_wallet.get_payments(matched_payments, actual_payments,
-9999, -1);
ASSERT_EQ(0, matched_payments.size());
}
TEST_F(GetPayments, NotFoundIfNegativeReversed_Out)
{
m_wallet.get_payments_out(confirmed_payments, actual_confirmed_txs,
-9999, -1);
ASSERT_EQ(0, confirmed_payments.size());
}


TEST_F(GetPayments, NotFoundIfMinHeightAboveMaxHeight)
{
m_wallet.get_payments(matched_payments, actual_payments,
max_height, min_height);
ASSERT_EQ(0, matched_payments.size());
}
TEST_F(GetPayments, NotFoundIfMinHeightAboveMaxHeight_Out)
{
m_wallet.get_payments_out(confirmed_payments, actual_confirmed_txs,
max_height, min_height);
ASSERT_EQ(0, confirmed_payments.size());
}


TEST_F(GetPayments, IsFoundWithinZeroAndMax)
{
m_wallet.get_payments(matched_payments, actual_payments,
min_height, max_height);
ASSERT_EQ(1, matched_payments.size());
}
TEST_F(GetPayments, IsFoundWithinZeroAndMax_Out)
{
m_wallet.get_payments_out(confirmed_payments, actual_confirmed_txs,
min_height, max_height);
ASSERT_EQ(1, confirmed_payments.size());
}


TEST_F(GetPayments, NotFoundAtMinHeight)
{
m_wallet.get_payments(matched_payments, actual_payments,
block_height_containing_transaction, block_height_containing_transaction);
ASSERT_EQ(0, matched_payments.size());
}
TEST_F(GetPayments, NotFoundAtMinHeight_Out)
{
m_wallet.get_payments_out(confirmed_payments, actual_confirmed_txs,
block_height_containing_transaction, block_height_containing_transaction);
ASSERT_EQ(0, confirmed_payments.size());
}


TEST_F(GetPayments, IsFoundAtMaxHeight)
{
m_wallet.get_payments(matched_payments, actual_payments,
min_height, block_height_containing_transaction);
ASSERT_EQ(1, matched_payments.size());
}
TEST_F(GetPayments, IsFoundAtMaxHeight_Out)
{
m_wallet.get_payments_out(confirmed_payments, actual_confirmed_txs,
min_height, block_height_containing_transaction);
ASSERT_EQ(1, confirmed_payments.size());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add more of them ? At the moment it's just one, does not test much.

Copy link
Author

Choose a reason for hiding this comment

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

This should do: e65fd7f

I cleaned the structure for the tests and added several new variants, all of which are now also implemented for the get_payments_out function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's just one payment in the list. I'd expect a test to test when there are no payments, more than one payment per height, payments both in and out of the range you query, etc.