-
Notifications
You must be signed in to change notification settings - Fork 58
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
Json rpc/non sensitive stakes ranks utxos #2570
Json rpc/non sensitive stakes ranks utxos #2570
Conversation
c931032
to
c2cb559
Compare
Could you rename the commit 15d0e82 to be less than 72 chars to avoid truncation? I propose something like "feat(json_rpc): fetch ranked stake entries by capability" |
Regarding the JSONRPC method "getUtxoInfo", I don't see why it is a sensitive as it requires a pkh. The sensitive layer lies in the CLI where uses the node's pkh as the default value. |
Feel free to request a review when ready. |
… a given capability
As input param `pkh` is mandatory.
c2cb559
to
32f9330
Compare
Exactly |
node/src/actors/json_rpc/api.rs
Outdated
@@ -145,6 +145,12 @@ pub fn attach_regular_methods<H>( | |||
server.add_actix_method(system, "queryStakes", |params: Params| { | |||
Box::pin(query_stakes(params.parse())) | |||
}); | |||
server.add_actix_method(system, "ranks", |params: Params| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just nitpicking here. What you get in return of this method is a list comprising all stake entries, ranked by power. Let me argue that's a "ranking" and not a "ranks" inasmuch it's not a list of ranks (like in military ranks, etc) but more of a ranked / sorted kind of roster.
server.add_actix_method(system, "ranks", |params: Params| { | |
server.add_actix_method(system, "ranking", |params: Params| { |
Alternatively, if we follow the pattern of naming the method with the action it will perform instead of what we are expecting it to return (like in methods named getX
or queryX
), then rank
could make sense too:
server.add_actix_method(system, "ranks", |params: Params| { | |
server.add_actix_method(system, "rank", |params: Params| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, perhaps a better name would be "queryPowers":
- as queryStakes was not nitpicked
- and because this new method doesn't actually return a list of stake entries, but tuples containing the validator, withdrawer and staking power at present time, for specified capability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is 🔝
No description provided.