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

Json rpc/non sensitive stakes ranks utxos #2570

Closed

Conversation

guidiaz
Copy link
Contributor

@guidiaz guidiaz commented Jan 13, 2025

No description provided.

@guidiaz guidiaz force-pushed the json_rpc/non-sensitive-stakes-ranks-utxos branch from c931032 to c2cb559 Compare January 13, 2025 08:09
@Tommytrg
Copy link
Member

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"

image

@Tommytrg
Copy link
Member

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.

@aesedepece
Copy link
Member

Feel free to request a review when ready.

@guidiaz guidiaz force-pushed the json_rpc/non-sensitive-stakes-ranks-utxos branch from c2cb559 to 32f9330 Compare January 14, 2025 15:10
@guidiaz
Copy link
Contributor Author

guidiaz commented Jan 14, 2025

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.

Exactly

@@ -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| {
Copy link
Member

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.

Suggested change
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:

Suggested change
server.add_actix_method(system, "ranks", |params: Params| {
server.add_actix_method(system, "rank", |params: Params| {

Copy link
Contributor Author

@guidiaz guidiaz Jan 15, 2025

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

@guidiaz guidiaz requested a review from aesedepece January 16, 2025 11:55
Copy link
Member

@aesedepece aesedepece left a comment

Choose a reason for hiding this comment

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

This is 🔝

@aesedepece aesedepece closed this Jan 19, 2025
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.

3 participants