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

Suggestions for RPC doc enhancements #620

Closed
paulmelis opened this issue Feb 15, 2018 · 9 comments

Comments

5 participants
@paulmelis
Copy link
Contributor

commented Feb 15, 2018

Here's some notes I made while working locally with the RPC interface. I'm not doing any kind of exchange integration, merely trying some stuff locally.

  • A more precise description of what enable_control enables would be handy, as it currently is a bit implicit. For example, in RaiBlocks_Exchange_Integration_Guide_rev1.pdf it says

    In the node’s config.json file both “rpc_enable” and “enable_control” must be set to “true” for wallet control and accounting. If using the node only as a RaiBlocks network interface, “enable_control” can be “false”.

    What exactly does "accounting" mean here? There's quite a few account-related calls that can be run without enable_control. But, for example, the "ledger" call does require enable_control, but it seems like a query-only call without side-effects. Or is this because of the node-specific data it returns?

  • The count parameter that some calls take apparently can be set to -1, to get all results, but this isn't mentioned anywhere.

  • Where exactly does one get a wallet ID from (other than the one returned when creating with wallet_create)? There doesn't seem to be a wallet_list command to get IDs of existing wallets.

  • The "process" call's description is "Publish block to the network". Does it merely publish the block, or does it also do some kind of processing on it (as the name suggests)?

  • Under https://github.com/nanocurrency/raiblocks/wiki/RPC-protocol#retrieve-node-versions, spelling mistake "retruns"

@SergiySW

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2018

"count": "0" should return 0 results, what RPC are you mentioning?
curl -g -d '{ "action": "ledger", "count": "0"}' [::1]:7076
{
"accounts": ""
}

@SergiySW

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2018

There is CLI --wallet_list, for security reasons it's not exposed in RPC #574

@paulmelis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2018

@SergiySW, I edited the count remark to say that you can set it to -1 to get all results

@SergiySW

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2018

RPC ledger is most CPU & bandwidth insentive. It was the only reason to protect it with enable_control actually.

@paulmelis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2018

Just noticed the added description of the id parameter with the send call. Part of the description seems a bit ambiguous:

If the request times out, then the send may or may not have gone through. Most exchange "double withdraw" bugs are caused because a request was incorrectly retried. If you want to retry a failed send, you must specify the id parameter as follows:

This reads as if adding the id is only needed if you want to retry a send, while you should obviously then already have added the id on the original send call. The description below it on the parameter itself is pretty clear in this respect, btw. In general I'd say it should be made very clear that you should add the id parameter on all send calls or don't use it all. Also, the word "must" here is a bit weird, as use of id is not enforced yet it seems. It's merely a recommendation for doing things robustly, so "strongly recommended" would be better.

If you accidentally reuse an id, the send will not go through (it will be seen as a duplicate request), so make sure your ids are unique! They must be unique per node, and are not segregated per wallet.

Using the same id for requests with different parameters (wallet, source, destination, and amount) is undefined behavior and may result in an error in the future.

These two sentences are in conflict, as "accidentally reuse an id" is the same as "same id for requests with different parameters". For the former the "send will not go through" (defined behaviour), for the latter it "is undefined behaviour".

Final note: are the send ids stored persistently? Or are they only valid per node session? Might be useful to know in terms of memory usage of using very large ids over a long time.

@cryptocode

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2018

@paulmelis think the first part is solved simply rephrasing to "If you want to the ability to retry a failed send, all send calls must specify the id parameter as follows:" or something to that effect.

@PlasmaPower I think tweaking the docs here is a good idea.

for the latter it "is undefined behaviour"

It says it may be undefined behavior in the future, not that it is.

Final note: are the send ids stored persistently?

The id's are stored and they must be unique across wallets.

@paulmelis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2018

for the latter it "is undefined behaviour"

It says it may be undefined behavior in the future, not that it is.

Errr:

Using the same id for requests with different parameters (wallet, source, destination, and amount) is undefined behavior

@paulmelis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2018

The example given for validate_account_number does not match the actual result. According to the docs should return 1, but returns 0 in practice:

$ curl -X POST -d '{"action":"validate_account_number", "account": "xrb_3e3j5tkog48pnny9dmfzj1r16pg8t1e76dz5tmac6iq689wyjfpi00000000"}' http://localhost:7076
{
    "valid": "0"
}

Btw, I assumed the wiki pages were only editable by core devs, especially things like the RPC docs, but it seems they're editable by everyone. Is that wise? Why not include crucial stuff that needs to be under control, like the RPC docs, in the repo?

@rkeene rkeene added this to the V19.0 milestone Aug 23, 2018

@zhyatt zhyatt self-assigned this Feb 19, 2019

@zhyatt zhyatt added this to CP2 in V19 Feb 19, 2019

@zhyatt

This comment has been minimized.

Copy link
Collaborator

commented Mar 25, 2019

Closing as updates made previously.

@zhyatt zhyatt closed this Mar 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.