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

RPC Help strings : (requires assetindex to be enabled) #1186

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

Conversation

sLiinuX
Copy link

@sLiinuX sLiinuX commented Mar 10, 2022

Added the mention (requires assetindex to be enabled) into the help return string of the two following rpc calls :

  • listassetbalancesbyaddress
  • listaddressesbyasset

RPC calls return an error anyway but better to mention this requirement in the help string as well.
Usefull for devs and users that search for a function and requirements for it.

Added the mention (requires assetindex to be enabled) into the help return string of the two following rpc calls :
- listassetbalancesbyaddress
- listaddressesbyasset 

RPC calls return an error anyway but better to mention this requirement in the help string as well.
Usefull for devs and users that search for a function and requirements for it.
Copy link

@NiftyRaven NiftyRaven left a comment

Choose a reason for hiding this comment

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

Review for Pull Request on src/rpc/assets.cpp:


Firstly, I want to express my appreciation for the work put into refining the RPC documentation in src/rpc/assets.cpp. It’s clear that these changes aim to improve clarity for users interacting with asset balances and addresses, which is fantastic.

The modification to explicitly mention that assetindex needs to be enabled for listassetbalancesbyaddress and listaddressesbyasset to function properly is particularly valuable. This addition helps to set clear expectations for users and potentially reduces confusion and support queries related to these RPC calls.

Feedback on Changes:

  • The repetition of the phrase "Returns a list of all asset balances for an address" and similar for the other function before specifying the assetindex requirement might be streamlined for brevity and clarity. Perhaps, we could merge these into a single, concise sentence that incorporates the requirement upfront. For example:
    • "Returns a list of all asset balances for an address, requiring assetindex to be enabled."
  • Additionally, for listaddressesbyasset, the phrase "Or returns the total size of how many address own the given asset" introduces a slightly different functionality. It might be helpful to clarify whether this is an additional outcome based on the provided arguments (like onlytotal) or if it's part of the general function behavior. If it's dependent on specific arguments, explicitly stating this relationship could enhance understanding.

Suggestions:

  • Including example command usages for these RPC calls in the comments might further assist users in understanding how to use these features effectively. Providing practical examples can be incredibly helpful for both new and seasoned users of the system.
  • It would be beneficial to include information or a reference on how to enable assetindex for users who might not be familiar with this requirement.

Overall, these updates significantly contribute to the usability and accessibility of Ravencoin’s RPC functionality. I'm looking forward to these improvements being merged, enhancing the developer and user experience within the Ravencoin ecosystem.

Thank you for your dedication to refining and improving the documentation and functionality of Ravencoin’s RPC interface. Your efforts are sincerely appreciated by the community.

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

Successfully merging this pull request may close these issues.

2 participants