request: wallet/api/unsigned_transaction.cpp deprecate recipientAddress() for confirmationMessage()

[plowsof]

monero/src/wallet/api/unsigned_transaction.cpp

Line 300 in 0f84863

result.push_back(cryptonote::get_account_address_as_str(m_wallet.m_wallet->nettype(), utx.dests[0].is_subaddress, utx.dests[0].addr));

when testing offline signer wallets - i noticed cupcake had an issue with a pay-to-many transaction. they have a check to confirm (amts.length != addrs.length) but are only given dests[0] where as amount returns all

anonero also uses recipientAddress() . anonero-fork has provided a solution to move to pay-to-many

reason for deprecation is this could lead to a huge gotcha bug where consumers of the api using recipientAddress() only show dest[0] of a pay to many transaction - possibly leading to the cold wallet being drained. cupcake have protections against this but anonero does not (these are universal* cold signers so could be presented a pay-to-many transaction from a compatible wallet)

monero-gui and feather are looping over the recipientAddress() is dead code?

monero-gui uses m_confirmationMessage here which contains all the destinations just fine.

[SNeedlewoods]

As you identified recipientAddress() is unused by monero-wallet-gui and to ensure the GUI shows all recipients when signing a UnsingedTransaction it uses confirmationMessage().
AFAICT, if we would remove recipientAddress() we would "force" Wallet API users to use confirmationMessage(), which is just one long string. IMO the pro for confirmationMessage(), that it also contains info about fee and change (otherwise there is currently no way to get the change amount from an UnsignedTransaction, but that function would be easy to add if needed) is outweighed by the con to force everyone to use this exact format, or having to deal with string manipulation to display only what they want.
Therefore I'd prefer to change recipientAddress() to return all recipients, to give Wallet API users more fine grained control:

std::vector<std::string> UnsignedTransactionImpl::recipientAddress() const 
{
    std::vector<string> result;
    for (const auto &utx: m_unsigned_tx_set.txes) {
        if (utx.dests.empty()) {
          MERROR("empty destinations, skipped");
          continue;
        }
-        result.push_back(cryptonote::get_account_address_as_str(m_wallet.m_wallet->nettype(), utx.dests[0].is_subaddress, utx.dests[0].addr));
+        for (const auto &u_dest : utx.dests)
+          result.push_back(cryptonote::get_account_address_as_str(m_wallet.m_wallet->nettype(), u_dest.is_subaddress, u_dests.addr));
    }
    return result;
}

[selsta]

+1 for changing recipientAddress like @SNeedlewoods suggested and reaching out to all wallets that are known to use recipientAddress

[plowsof]

yes +1 for @SNeedlewoods fix

reaching out to all wallets that are known to use recipientAddress

i can put this on my todo list, or at least, first checking some wallets in the ecosystem, other than anonero and cupcake.