Skip to content
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

src: add net type and daemon address to wallet rpc #9406

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

D3vN1
Copy link

@D3vN1 D3vN1 commented Jul 26, 2024

Copy link
Collaborator

@selsta selsta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please bump WALLET_RPC_VERSION_MINOR

@D3vN1
Copy link
Author

D3vN1 commented Jul 28, 2024

@selsta I've increased WALLET_RPC_VERSION_MINOR from 27 to 28

@0xFFFC0000
Copy link
Collaborator

Please squash two commits. And push it as a same commit.

@D3vN1 D3vN1 force-pushed the dev/d3vn1/add-url-and-nettype-wallet-rpc branch from dbcbb78 to 56ca0a3 Compare July 28, 2024 20:28
@D3vN1
Copy link
Author

D3vN1 commented Jul 28, 2024

I have successfully squashed the two commits and pushed them as a single commit, as per your request.
Please let me know if there are any further changes needed.
Thanks

@hyc
Copy link
Collaborator

hyc commented Jul 28, 2024

Why do clients need to know this info?

{
std::string daemon_address;
std::string nettype;
bool mainnet;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It rarely makes sense to include outputs of pure functions of response data in a response; exceptions can be made in case of complicated functions, but these boolean values are all trivially derived from the nettype (as can be seen by the code which sets them).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants