provide better documentation for marker nav cmd. #2159
Conversation
WalkthroughThe changes in this pull request update the CLI command for adding or updating marker net asset values in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
x/marker/client/cli/tx.go (4)
1056-1058: Improve clarity of command usageThe updated
Usefield is more informative, but it could be even clearer.Consider updating the
Usefield to:Use: "add-net-asset-values <denom> <valuation> [<valuation>...]",This format more clearly indicates that at least one valuation is required, and multiple valuations are optional.
1059-1059: Enhance the short descriptionThe short description has been improved, but it could provide more context.
Consider expanding the short description to:
Short: "Provide net asset values for a marker, allowing for multiple valuations",This gives users a bit more information about the command's capabilities at a glance.
1060-1086: Approve and suggest improvements for the long descriptionThe addition of a detailed long description is a significant improvement. However, there are a few areas where it could be enhanced for clarity.
Consider the following improvements:
- Add a brief explanation of what net asset values are used for at the beginning.
- Clarify the relationship between "price" and "volume" in the valuation ratio.
- Explain why USD is represented in mils.
- Add a note about the importance of using whole integers for all values.
Here's a suggested revision:
Long: ` Provide net asset valuations for a marker. Net asset values are used to establish the relative value of the marker in relation to other assets or currencies. Net asset values are expressed as a ratio between an amount of coin paid (price) and a volume of the marker's tokens which are considered equivalent in value. The denomination of the amount paid (price) must either: 1) Exist on-chain as a separate marker, or 2) Be supplied as a [usd] integer valued in mils (1000 mils = $1 USD). The volume is supplied as an integer count of the current marker's tokens. IMPORTANT: All values must be represented as whole integers. If a decimal value is required, adjust the ratio between the price and volume to achieve the desired precision. `,This revision provides more context and clarity on the purpose and usage of the command.
1067-1086: Approve expanded examples with minor formatting suggestionThe expanded examples are excellent and provide clear guidance for various use cases.
To improve readability, consider adding empty lines between each example and its description. For instance:
Example: fmt.Sprintf(` Set a value of $1 = 1markercoin (Note USD is denominated in mils) $ %[1]s tx %[2]s add-net-asset-values markercoin 1000usd,1 Provide more than one valuation in a single call $ %[1]s tx %[2]s add-net-asset-values markercoin 1000usd,1;5000000000nhash,1 Valuations for larger trades with volumes greater than 1 $ %[1]s tx %[2]s add-net-asset-values markercoin 100000usd,199;1000stake,19 All values must be represented as whole integers. If a decimal value is required then the ratio between the price and volume must be adjusted to achieve the decimal required. Note: When the valuations are recorded, each will indicate the address of the admin who provided the value. This will be published in the associated event data and captured in the NAV record. For NAVs set by other modules such as x/exchange the protocol will indicate these sources. This separates established values from the owner (self-attestation) from those set through blockchain transactions. `,This formatting makes it easier to distinguish between different examples and their explanations.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/marker/client/cli/tx.go (1 hunks)
Additional comments not posted (1)
x/marker/client/cli/tx.go (1)
1056-1087: Approve changes to GetCmdAddNetAssetValues functionThe updates to the
GetCmdAddNetAssetValuesfunction significantly improve the documentation for theadd-net-asset-valuescommand. The changes align well with the PR objectives, providing clearer guidance on command usage, more detailed explanations, and expanded examples.While I've suggested some minor improvements for even better clarity and readability, the overall changes are a substantial improvement and address the documentation issues mentioned in the linked GitHub issue.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/marker/client/cli/tx.go (1 hunks)
Additional comments not posted (1)
x/marker/client/cli/tx.go (1)
1058-1095: Documentation enhancements enhance command clarityThe detailed explanations and additional examples in the
Longdescription significantly improve the user's understanding of theadd-net-asset-valuescommand. This will help users utilize the command effectively.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .changelog/unreleased/improvements/2128-marker-nav-docs.md (1 hunks)
Additional comments not posted (1)
.changelog/unreleased/improvements/2128-marker-nav-docs.md (1)
1-1: Changelog entry location and format are appropriate.The location of the changelog entry in the
.changelog/unreleased/improvements/directory and the file naming convention including the issue number are correct. The Markdown format is also appropriate for changelog entries.The structure and placement of this changelog entry align with best practices for maintaining a clear and organized changelog.
Description
Adds documentation for the marker NAV command.
closes: #2128
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments..changelog/unreleased(see Adding Changes).Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit
New Features
Documentation