[WIP] add functions to wallet API #9464
Conversation
|
As we can see from the table above, there are quite some things that I think we should not just rush headlong into adding those many things. After all, if we are successful, this API will be very important, will live for a long time, and will be used by many people. If the new resulting interface is not well designed, it will haunt us for years to come. Problem is that there are too many ways how you could design it, it's not "naturally clear" what types and methods there have to be. My feeling is that you could do almost everything in a hundred different ways. To contribute something useful in these quite difficult circumstances I try to come up with some general rules to follow when defining the API that will hopefully guide us towards a good solution. |
|
Make it complete This rule is pretty self-evident and clear, but I repeat it here nevertheless in the sake of completeness. Goal is to provide a viable successor to the "API" that the public things in Because of this, we have to make sure that the Wallet API is feature complete. We must not lose any wallet functionality that is needed and in actual use. |
|
Make it small We should try, while respecting the first rule of completeness of course, to make the API small. The smaller it is, the faster will new devs be able to read into it, the easier it will be to find the right methods to use, the less work to document it, etc. Where do we have potential to reduce the number of methods? One way I see is this: Do not include methods from I have mentioned an example of such method in this earlier issue about wallet related methods: There is a variant of Of course use common sense: It may be essential that already the code of the Wallet API filters or sorts something because otherwise too much data must get transferred, or doing things in the client would be much, much slower. |
|
Be careful about terminology A good API uses a consistent terminology when naming things, and uses terms that are "expressive", hard to misunderstand and hard to confuse. IMHO quite a number of things in This motivated me to propose to ban the term transfer outright exactly because it's so easy to misunderstand and easy to confuse. I detailed my proposal here. Of course many things in the Wallet API already have the names they have, and we must let them stand and are not completely free how to name the new things we add. Still I see quite a degree of freedom left. |
|
Limit the influence of existing clients We have 2 main clients of Of course, striclty only seen from the point of view of that migration, the more similar the Wallet API is to the "old" I propose "No" as answer. I think having a good API is much more important than trying to optimize towards migration work on the CLI wallet and the wallet RPC server being small. That migration happens once and is then over for all times, whereas the Wallet API will go indefinitely into the future and (hopefully) shape tons of Monero related software that together trump any migration work. In detail that means we don't try to give each method of |
|
Keep the style consistent I write this down more for the sake of completeness, like the very first rule Make it complete: There is a clash in naming style between the Wallet API and most of the rest of the Monero codebase: The first uses casing to form names like For the things we add we basically have two choices: We keep the naming style consistent with the things that are already there in the Wallet API, or we use the snake casing that is much more widely used in the codebase. Both choices are well possible, and both have advantages as well as disadvantages. (Renaming existing things as a third option and a way to achieve overal naming style consistency is probably not on the table.) We dicussed this in a Seraphis wallet workgroup meeting, and it seemed that more people voted for keeping the style consistent. I myself was also in that camp: To continue with the "non-standard" naming style is IMHO the lesser evil than mixing styles within a single API. |
|
Now my opinion about one the more important design questions:
As I explained here, neither the Wallet API, nor the RPC interface, nor Woodser's wallet API have such separate calls. They basically work with a single call that get payments / transactions of all types, using a struct that is "encompassing" i.e. is able to store all details of all these types. I hope I got that right, the Wallet API call for this seems to be According to the rules Make it small and Limit the influence of existing clients I propose to continue with this one call and not create direct equivalents of these methods in the Wallet API. Probably the type |
src/wallet/api/wallet2_api.h
Outdated
| virtual bool isFrozen(const std::string multisig_sign_data) const = 0; | ||
| // QUESTION : Should we add the following function, which returns strings containing the key image for every frozen enote? I think it can be useful | ||
| // virtual std::vector<std::string> getAllFrozenEnotes() const = 0; | ||
| // TODO : Would this better fit in one of `Subaddress`/`SubaddressAccount` classes? |
There was a problem hiding this comment.
Can you give some possible arguments for pushing this down to the account level (if I understand you correctly)? I don't see any right now, but you have probably give this much more thought than I :)
There was a problem hiding this comment.
Quick update:
There is no need to move createOneOffSubaddress to the account level. Since this is an exotic function, maybe we should add a note/warning that this function should only be used in special cases?
(moneromoo: "It was intended to help people who generated a subaddress far into the random index values.")
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 adds accounts/subaddresses consecutively and keeps track of accounts and subaddresses.
WalletImpl::addSubaddress & WalletImpl::addSubaddressAccount also do not refresh the known accounts/subaddresses, if that's not for a good reason (that I don't see) I would either add refresh at the end of those functions, or at least give the same warning as proposed above.
Seems the GUI only uses the addRow functions (see here) not the addSubaddress* ones (see here).
src/wallet/api/wallet2_api.h
Outdated
| * brief: getTransfers - get all transfers | ||
| * outparam: transfers - | ||
| */ | ||
| // virtual void getTransfers(wallet2::transfer_container& transfers) const = 0; |
There was a problem hiding this comment.
Here we probably enter a whole region of things that we won't need in this form if my proposal to make a single encompassing "get payments / transactions" methods really flies.
src/wallet/api/wallet2_api.h
Outdated
| * brief: getMinRingSize - get the minimum allowed ring size | ||
| * return: min ring size | ||
| */ | ||
| virtual std::uint64_t getMinRingSize() const = 0; |
There was a problem hiding this comment.
Can go. We don't have the possibility to choose ringsize anymore for years, and well, soon we won't have any rings at all. Maximum that I see here is an info "ring size" in the new WalletState.
There was a problem hiding this comment.
Would require something like this: tobtoht@894adf0. Can PR.
src/wallet/api/wallet2_api.h
Outdated
| * messages and prompts. When it finally calls this method here "to catch up" so to say we can't use | ||
| * incremental update anymore, because with that we might miss some txs altogether. | ||
| */ | ||
| virtual void updatePoolState(std::vector<std::tuple<cryptonote::transaction, std::string, bool>> &process_txs, bool refreshed = false, bool try_incremental = false) = 0; |
There was a problem hiding this comment.
I am not sure yet what possibilities of "pool info update" the Wallet API has to offer, or should offer, but I am pretty sure that this method in this form is on too low a level of abstraction, like the following processPoolState as well.
There was a problem hiding this comment.
This is too complex for me right now to fully dive into, maybe someone with more experience in this area can comment?
There was a problem hiding this comment.
I try to follow the general principle of YAGNI, especially when making APIs. Don't add it unless someone asks for it. A use case for processing abstractions of the pool state might pop up in the future at some point, but it also might not. Until then, these functions are internal business logic and shouldn't be exposed to the API.
There was a problem hiding this comment.
If we don't add them to the API, how would the wallet-rpc & wallet-cli call this function, if the goal is to get rid of all the wallet2 function calls? Sorry if I'm missing something.
There was a problem hiding this comment.
I checked, and it's used indeed, so it's not a case of YAGNI like @jeffro256 suspected, and after I saw that @moneromooo-monero fixed an information leak a few years back when using remote daemons where somehow this method and the next for processing the pool state were involved, I think it's best to just add those to the interface and keep them "public".
There was a problem hiding this comment.
I think that the function update_pool_state, with process_pool_state are both leaky abstractions as written. Both times they are used outside of wallet2, they are called like so:
std::vector<std::tuple<cryptonote::transaction, crypto::hash, bool>> process_txs;
m_wallet->update_pool_state(process_txs);
if (!process_txs.empty())
m_wallet->process_pool_state(process_txs);
This sequence of statements basically means "rescan the pool" and IMO should be abstracted into a method as such. Calling update_pool_state without an immediate corresponding call to process_pool_state will almost always be a mistake, as this will update the fetching marker without actually scanning the transactions and getting up-to-date information about your balance. So I'd add a method to the Wallet interface called something like refreshPoolOnly which calls the above statements, and drop both updatePoolState and processPoolState from the interface.
| * return: true if succeeded | ||
| * note: sets status error on fail | ||
| */ | ||
| virtual bool saveMultisigTx(const PendingTransaction &multisig_ptx, const std::string &filename) const = 0; |
There was a problem hiding this comment.
I wonder whether it would make sense to group all the multisig related methods together in the source, and not e.g. put non-multisig and multisig versions of methods that do similar things together. Thus, "write normal tx to file" and "write multisig transaction to file" would not be next to each other, but the first in the group of "normal" methods and the second in the multisig method group.
My argument: Many wallet apps will probably continue to not offer multisig, and their devs may welcome that all those multisig methods do not "clutter" the API for them.
There also have been thoughts that ideally, "multisig wallet" would be a completely separate wallet type, with its own API, and maybe even with the code in a separate repository. That multisig stuff is so highly special, so different, and so complicated ...
I also ask myself wether we can't make some of such methods "smart" anyway so they decide themselves what there is to do, based on internal knowledge whether a transaction is multisig or not.
There was a problem hiding this comment.
In regards to grouping multisig stuff together, that's already done in another PR (can be seen e.g. here or here). If I remember correctly for this PR I was asked to add everything new in one place, if the "organize functions" get merged this can get based onto that (actually there is still another one for in between in the pipeline I didn't PR yet, the "add comments" found here).
A separate multisig wallet really sounds ideal, not sure how many clients currently use the multisig functionality from the API, the GUI afaik doesn't. But that's out of scope for now I'd say!?
I'll look into if I can come up with such "smart" methods.
src/wallet/api/wallet2_api.h
Outdated
| * brief: getAccountTags - get the list of registered account tags | ||
| * return: first.Key=(tag's name), first.Value=(tag's label), second[i]=(i-th account's tag) | ||
| */ | ||
| virtual const std::pair<std::map<std::string, std::string>, std::vector<std::string>>& getAccountTags() = 0; |
There was a problem hiding this comment.
Seems to me, if we "go with the pattern", we have a method somewhere that gives back all existing accounts with info, using an account_info struct. The tag would be part of that struct, and thus this call here not needed because not "essential".
Or maybe a call giving back all subaddresses, with info about them, and info how they group into accounts - and any tags of course.
Methods like the following setAccountTag stay of course even if we use this pattern whenever we can, because changing things is something fundamentally different and altogether impossible if there is no method, after all.
There was a problem hiding this comment.
I think that tags would fit into SubaddressAccountRow, but returning the "i-th account's tag" is just one part of this function, it also returns all tag descriptions (used by wallet-cli and wallet-rpc). So we probably need at least a getAccountTagDescriptions() method anyways, or alternatively we could store the tag description in SubaddressAccountRow, too.
(Method name: reason) signTxToStr: gets added in monero-project#9492 as `signAsString()` loadTx: not needed, all it does is `loadFromFile()` and `parseTxFromStr()` estimateBacklog(min_tx_weight, max_tx_weight, fees): not needed, calls `estimateBacklog(fee_levels)`
src/wallet/api/wallet2_api.h
Outdated
| * messages and prompts. When it finally calls this method here "to catch up" so to say we can't use | ||
| * incremental update anymore, because with that we might miss some txs altogether. | ||
| */ | ||
| virtual void updatePoolState(std::vector<std::tuple<cryptonote::transaction, std::string, bool>> &process_txs, bool refreshed = false, bool try_incremental = false) = 0; |
There was a problem hiding this comment.
I think that the function update_pool_state, with process_pool_state are both leaky abstractions as written. Both times they are used outside of wallet2, they are called like so:
std::vector<std::tuple<cryptonote::transaction, crypto::hash, bool>> process_txs;
m_wallet->update_pool_state(process_txs);
if (!process_txs.empty())
m_wallet->process_pool_state(process_txs);
This sequence of statements basically means "rescan the pool" and IMO should be abstracted into a method as such. Calling update_pool_state without an immediate corresponding call to process_pool_state will almost always be a mistake, as this will update the fetching marker without actually scanning the transactions and getting up-to-date information about your balance. So I'd add a method to the Wallet interface called something like refreshPoolOnly which calls the above statements, and drop both updatePoolState and processPoolState from the interface.
- add `refreshPoolOnly()` which handles `upadte_pool_state()` and `process_pool_state` - remove `cryptonote_basic.h` from interface - change `shared_ptr` to `unique_ptr`
SNeedlewoods
force-pushed
the
x_api_add_new_functions
branch
from
f9e2bcf
to
6704d55
Compare
UpdateTestsStarted working on tests on a new branch which helped finding the following minor issues with the Wallet API so far:
TODOHere are the last couple things which need attention and have been on my list for a while, but I haven't made any progress on recently, so feedback would be appreciated:
|
|
I had a look at the TODOs today. I am not sure how much my comments will help, but let me write them down nevertheless:
I agree with your decision to leave them out, at least for the time being. Maybe we will be forced to add them if one day we really swap out
As far as I can see you solved that in the meantime. I had a look at your latest, corresponding commit, and I think I could understand the flow of control more or less, and things look ok to me.
This is indeed only relevant in connection with the MMS, and the fate of that is pretty unclear. @tobtoht is currently building something that will replace / succeed it, and I don't have any knowledge how they will propose to modify and/or enlarge the interface of
I think a port of |
SNeedlewoods
force-pushed
the
x_api_add_new_functions
branch
from
c4488ab
to
e077fd3
Compare
|
Two more things:
|
Do not merge, this is still in development and just PRd for easier discussion.
This is an attempt to make the wallet API "feature complete" and it's part of this proposal.
The approach was to search for every
wallet2function insimplewalletandwallet_rpc_server, if it is used in either of those, but is not implemented in the API I considered it missing and it was added in this PR.I believe
wallet2_api.hnow contains all missing functions, but some are not implemented yet. I'm waiting on feedback on those, if you want to have a look, things I need help with are marked asQUESTION, you can also comment on things markedTODO(those may become QUESTIONs if I'm unable(/unsure how) to solve them) or anything else.Also gathered this collection of (mostly minor) issues found during API work.