Skip to content

feat(tendermint): staking queries#2377

Merged
shamardy merged 10 commits intodevfrom
staking-query-rpcs
Mar 19, 2025
Merged

feat(tendermint): staking queries#2377
shamardy merged 10 commits intodevfrom
staking-query-rpcs

Conversation

@onur-ozkan
Copy link
Copy Markdown

@onur-ozkan onur-ozkan commented Feb 27, 2025

Implements delegations and ongoing_undelegations searching RPCs including rewards and undelegation completion period information for tendermint.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan onur-ozkan changed the base branch from staking-rpcs to dev February 27, 2025 16:11
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan onur-ozkan force-pushed the staking-query-rpcs branch 4 times, most recently from f729bc0 to 47e4489 Compare March 3, 2025 10:40
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Copy link
Copy Markdown

@laruh laruh left a comment

Choose a reason for hiding this comment

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

I think it's better to follow common recommendations and implement the Display trait instead of a direct ToString implementation.

https://github.com/KomodoPlatform/komodo-defi-framework/blob/7bcf53389024c92b7df9ce77b351fef6751806ec/mm2src/coins/rpc_command/tendermint/staking.rs#L21-L30

impl Display for ValidatorStatus {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        let str = match self {
            // An empty string doesn't filter any validators and we get an unfiltered result.
            ValidatorStatus::All => String::default(),
            ValidatorStatus::Bonded => "BOND_STATUS_BONDED".into(),
            ValidatorStatus::Unbonded => "BOND_STATUS_UNBONDED".into(),
        };
        write!(f, "{}", str)
    }
}

@onur-ozkan
Copy link
Copy Markdown
Author

Could you elaborate how it's being the common recommendation to prefer Display trait for String castings?

@laruh
Copy link
Copy Markdown

laruh commented Mar 5, 2025

Could you elaborate how it's being the common recommendation to prefer Display trait for String castings?

https://doc.rust-lang.org/std/fmt/trait.Display.html

Implementing this trait for a type will automatically implement the ToString trait for the type, allowing the usage of the .to_string() method. Prefer implementing the Display trait for a type, rather than ToString.

As I understand implementing Display makes code easier to maintain and integrates with formatting macros.

@onur-ozkan
Copy link
Copy Markdown
Author

We don't need formatting on the type so it's unnecessary. We specifically and explicitly need ToString, anything extra is redundant IMO.

Comment on lines +2245 to +2257
Cosmos(rpc_command::tendermint::staking::SimpleListQuery),
}

#[derive(Deserialize)]
pub struct UndelegationsInfo {
pub coin: String,
info_details: UndelegationsInfoDetails,
}

#[derive(Debug, Deserialize)]
#[serde(tag = "type")]
pub enum UndelegationsInfoDetails {
Cosmos(rpc_command::tendermint::staking::SimpleListQuery),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is so much code in lp_coins.rs related to staking and delegation.

What about moving it to pub mod staking? The module can be inside the coins workspace.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah... I also hate that, but the thing is even with staking module in coins module it wouldn't be good enough, in fact, I think it will be even bad because it will look like we have duplicated staking modules (one from tendermint and one from coins) and it will look inconsistent with how we handle other RPCs (right now they all included in lp_coins).

I would prefer to re-write this module in general, rather than applying a workaround solution for specific logic.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think it will be even bad because it will look like we have duplicated staking modules

You can rename staking in tendermint or just add doc comment at the top of file

Copy link
Copy Markdown
Author

@onur-ozkan onur-ozkan Mar 6, 2025

Choose a reason for hiding this comment

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

If I do it in this PR, it will be very hard for others to review this PR (due to moving lots of things around). I will wait for others opinion on this.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think you can create an issue to move staking code from lp_coins to its own module before adding more staking implementations.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If I do it in this PR, it will be very hard for others to review this PR (due to moving lots of things around). I will wait for others opinion on this.

Agreed, we should never do refactors that involves moving code around in the same PR as new functionalities.

Copy link
Copy Markdown

@borngraced borngraced left a comment

Choose a reason for hiding this comment

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

well done. I have two minor 2 notes

Signed-off-by: onur-ozkan <work@onurozkan.dev>
@shamardy shamardy requested a review from borngraced March 17, 2025 11:21
@shamardy shamardy requested a review from laruh March 17, 2025 11:21
@onur-ozkan
Copy link
Copy Markdown
Author

Can you please finalize your reviews? @laruh @borngraced

borngraced
borngraced previously approved these changes Mar 17, 2025
Copy link
Copy Markdown

@borngraced borngraced left a comment

Choose a reason for hiding this comment

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

Great work!

laruh
laruh previously approved these changes Mar 17, 2025
Copy link
Copy Markdown

@laruh laruh left a comment

Choose a reason for hiding this comment

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

LGTM! please dont forget to create an issue #2377 (comment)

shamardy
shamardy previously approved these changes Mar 18, 2025
Copy link
Copy Markdown
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

LGTM! Only one nit.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan onur-ozkan dismissed stale reviews from shamardy, laruh, and borngraced via 70b217f March 19, 2025 06:04
@shamardy shamardy merged commit cb89300 into dev Mar 19, 2025
24 checks passed
@shamardy shamardy deleted the staking-query-rpcs branch March 19, 2025 11:49
dimxy pushed a commit that referenced this pull request Mar 20, 2025
* dev:
  feat(tendermint): staking queries (#2377)
dimxy pushed a commit that referenced this pull request Mar 21, 2025
* dev:
  fix(tx-history): fix unhandled IBC and HTLC events (#2385)
  improvement(dep-stack): replace deprecated `instant` dependency  (#2391)
  feat(tendermint): staking queries (#2377)
  refactor(eth): use trait addr_to_string method instead of old function (#2348)
  fix(ci): use correct rustup component syntax in fmt-and-lint job (#2390)
  refactor(tx-query): use TxSearchRequest for tx queries (#2384)
  refactor(tpu-v2): allow to skip p2p message with taker payment spend preimage for eth (#2359)
  feat(rpc): add is_success field to legacy MySwapStatusResponse (#2371)
  fix(key-derivation): use stored Argon2 parameters instead of default values (#2360)
  fix(tests): stabilize `tendermint_coin::test_claim_staking_rewards` (#2373)
  improvement(RPCs): group staking rpcs under a namespace (#2372)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants