API organize functions #9368
API organize functions #9368
Conversation
|
Can you add a bit more information what you are changing here? The diff is huge. |
SNeedlewoods
force-pushed
the
x_api_organize_functions
branch
from
80aba87
to
6f24730
Compare
|
It was discussed in #no-wallet-left-behind. If you look at these three links for example, you see the functions are out of order: The feedback was:
That's what this PR is about.. Hope that answers your question? |
|
If things go according to plan, this PR is the first in a row of commits that deal with the Wallet API in the In short: This course of action was discussed e.g. here in a meeting of the Seraphis Wallet workgroup. When looking closer at the code of the Wallet API @SNeedlewoods noticed that things in .h and .cpp file are partially out of order, and private method implementations can be found between public method implementation. See their comment with a good example. We think that this is unfortunate and can get in the way if we go on and significantly enlarge the API, and later also do an extensive rewrite of all wallet code that is now in Thus the plan to make this PR was born: We reorder in a very first step, but take care to make it no more painful than absolutely necessary. Thus this PR makes no other changes than reordering declarations and implementations and removing a few stray comment lines that are of no use, and exclusively deals with Wallet API files. Thus anything that violates these rules should be seen as an error in the PR that needs correcting. |
|
Thanks for the PR. The hardest part is reviewing all these files and making sure no unintended code changes has been happened. One thing I will try is to compile modules in both case, and the object file should be same if no code changes has happened. Though the changes is spread to many files, will make it harder. If anyone has a better idea for reviewing this, I am all ears. |
| bool ready = false; | ||
| if (!m_wallet->multisig(&ready) || !ready) { | ||
| m_status = Status_Error; | ||
| m_errorString = tr("The wallet must be in multisig ready state"); |
There was a problem hiding this comment.
There is a semantical change here. The original form is here [1]. This new style condition is not deterministically the same as the original form.
There was a problem hiding this comment.
Thanks for taking a look and good catch. This is not intentional and probably snuck in when rebased. Change was pretty recent 3a47cda
Regarding your comment below, this PR is already supposed to be the broken down version of a bigger/messier PR, so if you find anything that's not "ordering functions", it does not belong, but creeped in. Except removing some comments which would get removed with the next PR, because I thought having two PRs (first move comment, second remove comment) will also be unnecessary reviewers work.
|
My apologies for being a bummer. But we have to make sure that no semantical changes are inside this PR as @rbrunner7 mentioned. The size of the PR is huge. Maybe we can break into many PRs. First only and only whitespace, then some semantical changes if necessary [1]. |
SNeedlewoods
force-pushed
the
x_api_organize_functions
branch
from
306bd70
to
202b2fc
Compare
| @@ -96,6 +196,7 @@ bool UnsignedTransactionImpl::sign(const std::string &signedFileName) | |||
| return true; | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Looks like an unmotivated whitespace change to me, I don't think an additional empty line is in order here ...
| { | ||
| return m_unsigned_tx_set.txes.size(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Just a comment, maybe for fellow reviewers: There are 5 whitespace changes included in that big moved and partially reordered code block, 5 lines where trailing extra blanks where removed, which is IMHO a good thing.
Otherwise they are identical, as they should be (verified mechanically with a file comparison tool).
src/wallet/api/wallet_manager.cpp
Outdated
| return m_http_client.set_proxy(address); | ||
| } | ||
|
|
||
| // Static |
There was a problem hiding this comment.
Is this // Static a new, additional comment, or does it come from some "old" place? If it's new, it probably shouldn't be there.
There was a problem hiding this comment.
You're right, it's new and shouldn't be here. Will ask for general opinions on such "block separator" comments in today's meeting.
src/wallet/api/wallet2_api.h
Outdated
| */ | ||
| virtual bool verifyMessageWithPublicKey(const std::string &message, const std::string &publicKey, const std::string &signature) const = 0; | ||
|
|
||
| // Static |
There was a problem hiding this comment.
This // Static looks like new, and maybe not worth it. Comments will come in a second round, right?
src/wallet/api/wallet2_api.h
Outdated
| //! sets proxy address, empty string to disable | ||
| virtual bool setProxy(const std::string &address) = 0; | ||
|
|
||
| // Static |
There was a problem hiding this comment.
Another // Static one ...
There was a problem hiding this comment.
In this file also some whitespace changes complicate the mechanical comparison with a tool somewhat, but again that's ok, and removed trailing blanks progress, IMHO.
Otherwise no more logic changes, after the one detected by @0xFFFC0000 is corrected now.
This is a small first step towards the new API: #9308