Skip to content

Commit

Permalink
WIP rbrunner7 review round 2
Browse files Browse the repository at this point in the history
  • Loading branch information
SNeedlewoods committed Sep 9, 2024
1 parent 5089541 commit a621fc5
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 90 deletions.
20 changes: 20 additions & 0 deletions src/wallet/api/transaction_info.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,26 @@ class TransactionInfoImpl : public TransactionInfo
uint64_t m_confirmations;
uint64_t m_unlock_time;

// TODO : If it's certain that we need these, then add getter functions (to `TransactionInfo` in wallet2_api.h and `TransactionInfoImpl` in transaction_info.*) and add values to new members where applicable (e.g. in `TransactionHistoryImpl`)

// from unconfirmed_transfer_details
uint64_t m_change;
// QUESTION : Can someone confirm that we don't need m_dests, because we have:
// - amount = m_transfers[i].amount
// - address = m_transfers[i].address
// - is_subaddress = m_subaddrIndex[i] != 0
// I'm especially unsure about this assumption, wasn't able to verify it:
// - is_integrated = WalletImpl::integratedAddress(m_paymentid) == m_transfers[i].address
// And I haven't figured this one out yet
// - original =
std::vector<cryptonote::tx_destination_entry> m_dests;
enum { pending, pending_in_pool, failed, confirmed } m_tx_state;
// QUESTION : Any comments if we need this? Haven't investigated yet.
std::vector<std::pair<crypto::key_image, std::vector<uint64_t>>> m_rings; // relative

// from unconfirmed_payments
bool m_double_spend_seen;

friend class TransactionHistoryImpl;

};
Expand Down
52 changes: 6 additions & 46 deletions src/wallet/api/wallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1087,6 +1087,8 @@ bool WalletImpl::synchronized() const
bool WalletImpl::refresh()
{
clearStatus();
//TODO: make doRefresh return bool to know whether the error occured during refresh or not
//otherwise one may try, say, to send transaction, transfer fails and this method returns false
doRefresh();
return status() == Status_Ok;
}
Expand Down Expand Up @@ -3043,7 +3045,10 @@ void WalletImpl::createOneOffSubaddress(std::uint32_t account_index, std::uint32
WalletState WalletImpl::getWalletState()
{
WalletState wallet_state{};

wallet_state.is_deprecated = m_wallet->is_deprecated();
wallet_state.ring_size = 16;
wallet_state.daemon_address = m_wallet->get_daemon_address();

return wallet_state;
}
Expand All @@ -3053,7 +3058,7 @@ bool WalletImpl::hasUnknownKeyImages()
return m_wallet->has_unknown_key_images();
}
//-------------------------------------------------------------------------------------------------------------------
void WalletImpl::rewrite(const std::string &wallet_name, const std::string &password)
void WalletImpl::rewriteWalletFile(const std::string &wallet_name, const std::string &password)
{
clearStatus();

Expand Down Expand Up @@ -3093,11 +3098,6 @@ std::map<std::uint32_t, std::pair<std::uint64_t, std::pair<std::uint64_t, std::u
return m_wallet->unlocked_balance_per_subaddress(index_major, strict);
}
//-------------------------------------------------------------------------------------------------------------------
bool WalletImpl::isTransferUnlocked(std::uint64_t unlock_time, std::uint64_t block_height)
{
return m_wallet->is_transfer_unlocked(unlock_time, block_height);
}
//-------------------------------------------------------------------------------------------------------------------
void WalletImpl::updatePoolState(std::vector<std::tuple<cryptonote::transaction, std::string, bool>> &process_txs, bool refreshed, bool try_incremental)
{
clearStatus();
Expand Down Expand Up @@ -3283,26 +3283,6 @@ std::uint64_t WalletImpl::getBaseFee()
return m_wallet->get_dynamic_base_fee_estimate();
}
//-------------------------------------------------------------------------------------------------------------------
std::uint64_t WalletImpl::getMinRingSize()
{
if (useForkRules(HF_VERSION_MIN_MIXIN_15, 0))
return 16;
if (useForkRules(8, 10))
return 11;
if (useForkRules(7, 10))
return 7;
if (useForkRules(6, 10))
return 5;
if (useForkRules(2, 10))
return 3;
return 0;
}
//-------------------------------------------------------------------------------------------------------------------
std::uint64_t WalletImpl::adjustMixin(std::uint64_t mixin)
{
return m_wallet->adjust_mixin(mixin);
}
//-------------------------------------------------------------------------------------------------------------------
std::uint32_t WalletImpl::adjustPriority(std::uint32_t priority)
{
clearStatus();
Expand Down Expand Up @@ -3416,11 +3396,6 @@ void WalletImpl::setTxKey(const std::string &txid, const std::string &tx_key, co
}
}
//-------------------------------------------------------------------------------------------------------------------
std::string WalletImpl::getDaemonAddress()
{
return m_wallet->get_daemon_address();
}
//-------------------------------------------------------------------------------------------------------------------
std::uint64_t WalletImpl::getDaemonAdjustedTime()
{
clearStatus();
Expand Down Expand Up @@ -3528,21 +3503,6 @@ std::uint64_t WalletImpl::getBlockchainHeightByDate(std::uint16_t year, std::uin
return 0;
}
//-------------------------------------------------------------------------------------------------------------------
bool isSynced()
{
clearStatus();

try
{
return m_wallet->is_synced();
}
catch (const std::exception &e)
{
setStatusError((boost::format(tr("Failed to check if wallet is synced. Error: %s")) % e.what()).str());
}
return 0;
}
//-------------------------------------------------------------------------------------------------------------------
std::vector<std::pair<std::uint64_t, std::uint64_t>> WalletImpl::estimateBacklog(const std::vector<std::pair<double, double>> &fee_levels)
{
clearStatus();
Expand Down
7 changes: 1 addition & 6 deletions src/wallet/api/wallet.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,11 +247,10 @@ class WalletImpl : public Wallet
void createOneOffSubaddress(std::uint32_t account_index, std::uint32_t address_index) override;
virtual WalletState getWalletState() const = 0;
bool hasUnknownKeyImages() const override;
void rewrite(const std::string &wallet_name, const std::string &password) override;
void rewriteWalletFile(const std::string &wallet_name, const std::string &password) override;
void writeWatchOnlyWallet(const std::string &wallet_name, const std::string &password, std::string &new_keys_file_name) override;
std::map<std::uint32_t, std::uint64_t> balancePerSubaddress(std::uint32_t index_major, bool strict) const override;
std::map<std::uint32_t, std::pair<std::uint64_t, std::pair<std::uint64_t, std::uint64_t>>> unlockedBalancePerSubaddress(std::uint32_t index_major, bool strict) const override;
bool isTransferUnlocked(std::uint64_t unlock_time, std::uint64_t block_height) const override;
void updatePoolState(std::vector<std::tuple<cryptonote::transaction, std::string, bool>> &process_txs, bool refreshed = false, bool try_incremental = false) override;
void processPoolState(const std::vector<std::tuple<cryptonote::transaction, std::string, bool>> &txs) override;
std::string convertMultisigTxToStr(const PendingTransaction &multisig_ptx) const override;
Expand All @@ -262,15 +261,12 @@ class WalletImpl : public Wallet
// bool loadMultisigTxFromFile(const std::string &filename, PendingTransaction &exported_txs, std::function<bool(const PendingTransaction&)> accept_func) const override;
std::uint64_t getFeeMultiplier(std::uint32_t priority, int fee_algorithm) const override;
std::uint64_t getBaseFee() const override;
std::uint64_t getMinRingSize() const override;
std::uint64_t adjustMixin(std::uint64_t mixin) const override;
std::uint32_t adjustPriority(std::uint32_t priority) const override;
void coldTxAuxImport(const PendingTransaction &ptx, const std::vector<std::string> &tx_device_aux) const override;
// void coldSignTx(const std::vector<pending_tx>& ptx_vector, signed_tx_set &exported_txs, std::vector<cryptonote::address_parse_info> &dsts_info, std::vector<std::string> & tx_device_aux) const override;
// const wallet2::transfer_details &getTransferDetails(std::size_t idx) const override;
void discardUnmixableOutputs() override;
void setTxKey(const std::string &txid, const std::string &tx_key, const std::vector<std::string> &additional_tx_keys, const boost::optional<std::string> &single_destination_subaddress) override;
std::string getDaemonAddress() const override;
std::uint64_t getDaemonAdjustedTime() const override;
void setCacheDescription(const std::string &description) override;
std::string getCacheDescription() const override;
Expand All @@ -280,7 +276,6 @@ class WalletImpl : public Wallet
std::string exportOutputsToStr(bool all = false, std::uint32_t start = 0, std::uint32_t count = 0xffffffff) const override;
std::size_t importOutputsFromStr(const std::string &outputs_str) override;
std::uint64_t getBlockchainHeightByDate(std::uint16_t year, std::uint8_t month, std::uint8_t day) const override;
bool isSynced() const override;
std::vector<std::pair<std::uint64_t, std::uint64_t>> estimateBacklog(const std::vector<std::pair<double, double>> &fee_levels) const override;
std::vector<std::pair<std::uint64_t, std::uint64_t>> estimateBacklog(std::uint64_t min_tx_weight, std::uint64_t max_tx_weight, const std::vector<std::uint64_t> &fees) const override;
bool saveToFile(const std::string &path_to_file, const std::string &binary, bool is_printable = false) const override;
Expand Down
87 changes: 49 additions & 38 deletions src/wallet/api/wallet2_api.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,51 @@ enum NetworkType : uint8_t {
bool set;
};

/**
* brief: EnoteDetails - Container for all the necessary information that belongs to an enote
*/
struct EnoteDetails
{
// from seraphis legacy enote types
// Ko
rct::key onetime_address;
/// a
rct::xmr_amount amount;
/// C
rct::key amount_commitment;
/// enc(x)
rct::key encoded_amount_blinding_factor;
/// enc(a)
rct::key encoded_amount;
/// view_tag
crypto::view_tag view_tag;

// TODO : figure out if we need other members from transfer_details too, these are the ones that are not part of `TransactionInfo`, but my first impression is we could have an overlap so we can find TransactionInfo from EnoteDetails and vice versa.
// from transfer_details:
cryptonote::transaction_prefix m_tx;
// index in m_tx.vout
uint64_t m_internal_output_index;
uint64_t m_global_output_index;
bool m_spent;
bool m_frozen;
uint64_t m_spent_height;
crypto::key_image m_key_image;
rct::key m_mask;
bool m_rct;
bool m_key_image_known;
bool m_key_image_request; // view wallets: we want to request it; cold wallets: it was requested
// TODO : fwiw so far this only is used for get_tx_pub_key_from_extra, figure out if actually needed.
uint64_t m_pk_index;
// TODO : This gets only filled by `wallet2::process_new_transaction` and only if `wallet2::m_track_uses` is true (default is false). Figure out if it's "too exotic" or do we need it.
std::vector<std::pair<uint64_t, crypto::hash>> m_uses;

// QUESTION : Any input on these multisig members?
// Multisig
bool m_key_image_partial;
std::vector<rct::key> m_multisig_k;
std::vector<multisig_info> m_multisig_info; // one per other participant
};

/**
* @brief Transaction-like interface for sending money
*/
Expand Down Expand Up @@ -456,6 +501,8 @@ struct Wallet

struct WalletState {
bool is_deprecated;
std::uint64_t ring_rize;
std::string daemon_address;
};

virtual ~Wallet() = 0;
Expand Down Expand Up @@ -1162,7 +1209,6 @@ struct Wallet
* note: sets status error on fail
*/
virtual std::string getMultisigSeed(const std::string &seed_offset) const = 0;
// TODO : Would this better fit in one of `Subaddress`/`SubaddressAccount` classes?
/**
* brief: getSubaddressIndex - get major and minor index of provided subaddress
* param: address - main- or sub-address to get the index from
Expand Down Expand Up @@ -1206,10 +1252,6 @@ struct Wallet
virtual bool isFrozen(const std::string &key_image) const = 0;
virtual bool isFrozen(const PendingTransaction &multisig_ptxs) const = 0;
virtual bool isFrozen(const std::string multisig_sign_data) const = 0;
// QUESTION : Should we add a note/warning that this should only be used in special cases? (Also addSubaddress & addSubaddressAccount do not refresh the known account/subaddress, if that's not for a good reason (that I don't see) I would either add refresh or at least give the same warning like the second line from the comment below. Seems the GUI only uses the addRow functions: https://github.com/search?q=repo%3Amonero-project/monero-gui%20addRow&type=code not the addSubaddress* ones https://github.com/search?q=repo%3Amonero-project/monero-gui%20addSubaddress&type=code ) e.g. something like:
// brief: createOneOffSubaddress - create a new account or subaddress for given index without adding it to known accounts and subaddresses.
// use `SubaddressAccount::addRow()` or `Subaddress::addRow()` instead to make sure the wallet keeps track of accounts and subaddresses

/**
* brief: createOneOffSubaddress - create a subaddress for given index
* param: account_index - major index
Expand All @@ -1228,12 +1270,12 @@ struct Wallet
*/
virtual bool hasUnknownKeyImages() const = 0;
/**
* brief: rewrite - rewrite the wallet file for wallet update
* brief: rewriteWalletFile - rewrite the wallet file for wallet update
* param: wallet_name - name of the wallet file (should exist)
* param: password - wallet password
* note: sets status error on fail
*/
virtual void rewrite(const std::string &wallet_name, const std::string &password) = 0;
virtual void rewriteWalletFile(const std::string &wallet_name, const std::string &password) = 0;
// QUESTION : Should we change this function from the current behavior in wallet2, so `wallet_name` is just the name of the new wallet instead of changing the `m_wallet_file` for the current wallet?
/**
* brief: writeWatchOnlyWallet - create a new watch-only wallet file with view keys from current wallet
Expand All @@ -1257,14 +1299,6 @@ struct Wallet
* return: [minor subaddress index: {amount, {blocks to unlock, time to unlock} }, ... ]
*/
virtual std::map<std::uint32_t, std::pair<std::uint64_t, std::pair<std::uint64_t, std::uint64_t>>> unlockedBalancePerSubaddress(std::uint32_t index_major, bool strict) const = 0;
// QUESTION : Can anyone help with this comment? There are other ones I'm not satisfied with, so if you have a better suggestion I'll change them.
/**
* brief: isTransferUnlocked - check if transfer is unlocked
* param: unlock_time -
* param: block_height -
* return: true if transfer is unlocked, else false
*/
virtual bool isTransferUnlocked(std::uint64_t unlock_time, std::uint64_t block_height) const = 0;
/**
* brief: updatePoolState -
* outparam: process_txs - [ [tx, tx_id, double_spend_seen], ... ]
Expand Down Expand Up @@ -1429,18 +1463,6 @@ struct Wallet
*/
virtual std::uint64_t getBaseFee() const = 0;
/**
* brief: getMinRingSize - get the minimum allowed ring size
* return: min ring size
*/
virtual std::uint64_t getMinRingSize() const = 0;
// TODO : Should we make this static?
/**
* brief: adjustMixin - adjust mixin to fit into min and max ring size
* param: mixin -
* return: adjusted mixin
*/
virtual std::uint64_t adjustMixin(std::uint64_t mixin) const = 0;
/**
* brief: adjustPriority - adjust priority depending on how "full" last N blocks are
* param: priority -
* return: adjusted priority
Expand Down Expand Up @@ -1498,11 +1520,6 @@ struct Wallet
// If we add access to boost::optional revisit all new functions to see which would benefit from it.
virtual void setTxKey(const std::string &txid, const std::string &tx_key, const std::vector<std::string> &additional_tx_keys, const boost::optional<std::string> &single_destination_subaddress) = 0;
/**
* brief: getDaemonAddress -
* return: daemon address
*/
virtual std::string getDaemonAddress() const = 0;
/**
* brief: getDaemonAdjustedTime -
* return: daemon adjusted time
* note: sets status error on fail
Expand Down Expand Up @@ -1562,12 +1579,6 @@ struct Wallet
* note: sets status error on fail
*/
virtual std::uint64_t getBlockchainHeightByDate(std::uint16_t year, std::uint8_t month, std::uint8_t day) const = 0;
/**
* brief: isSynced -
* return: true if wallet is synced with daemon, else false
* note: sets status error on fail
*/
virtual bool isSynced() const = 0;
// QUESTION : Can anyone help with these comments?
/**
* brief: estimateBacklog -
Expand Down

0 comments on commit a621fc5

Please sign in to comment.