-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
API organize functions #9368
base: master
Are you sure you want to change the base?
API organize functions #9368
Conversation
Can you add a bit more information what you are changing here? The diff is huge. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]. |
306bd70
to
202b2fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must be the dumbest review I ever did :)
But everything is ok, I wasn't able to unearth a single true code change. For a small number of questionable other changes I left comments. After those are addressed I am ready to approve.
@@ -96,6 +196,7 @@ bool UnsignedTransactionImpl::sign(const std::string &signedFileName) | |||
return true; | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another // Static
one ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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