Skip to content

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