feat(rpc): implement ledger_getEvents

[rkrasiuk]

Description

Implement ledger_getEvents through the extension trait. Not a fan of this solution but it saves you from the pain of having to handroll your own rpc macro.

Linked Issues

• Fixes Add ledger_getEvents to sov_ledger_rpc::RpcClient #1037

Testing

Added ledger_getEvents to getters_succeed test

[codecov]

Codecov Report

Merging #1058 (847acde) into nightly (27ae049) will increase coverage by 78.9%.
Report is 1 commits behind head on nightly.
The diff coverage is 100.0%.

Files	Coverage Δ
full-node/sov-ledger-rpc/src/client.rs	100.0% <100.0%> (ø)
full-node/sov-ledger-rpc/src/server.rs	95.5% <100.0%> (ø)

... and 203 files with indirect coverage changes

[neysofu]

Hi @rkrasiuk, thanks for the PR! Greatly appreciated.

The solution looks good. The extra trait approach is quite painful, although less than rolling our own hand-written client trait.

Sooner or later, I'd still like us to roll our own trait because it allows up to completely abstract away other serialization details (e.g. HexHash, there's no good reason for that to be part of the public crate API). In the meantime, maybe there we can avoid having the extra trait by extending the server implementation to accept both parameter types: the correct one, and jsonrpsee's auto-generated one. I have tried writing an MVP here: nightly...filippo/get-events-rpc.

What do you think? I'm unsure myself what I prefer.

[rkrasiuk]

hey @neysofu, i'm a bit at the crossroads as well. my preference would be for rpc to have strict behavior, but i like the code for your solution more, so i'll leave it up to you to decide :)

on the sidenote, could you point me to some good first issues i could take? very few seem to be tagged

[neysofu]

It's great that you want to pick up more issues, @rkrasiuk, thank you! I'll chat with the team later today and we'll tag a few issues as good-first-issue or something of that sort to help newcomers, and I can ping you when we do so.

As for the ledger_getEvents pattern, I think the nested parameter hack is good enough for now – when we hand-roll our own client trait, we can then remove the pathological case handler on the server's side, and the test for it.

Let me know if you're able to rewrite the branch history to include the changes in d86f02d. Feel free to modify the test, comments, or anything that you'd like to improve on. In the meantime, I'll create a new issue to track the hand-written client trait development.

[rkrasiuk]

no need to cherry-pick then, that solution looks good enough. closing in favor of d86f02d

[neysofu]

Hey @rkrasiuk!

I think there's few issues you could take on:

• Rewrite sov_ledger_rpc::client::RpcClient without jsonrpsee macros #1066 which is actually what we discussed here, i.e. hand-writing our own client trait with proper serialization and hiding details from the API such as HexString.
• rename associated type "Config" in the module trait to something that better illustrates its use #544
• There's no issue for this one (yet), but basically all of our modules (as in, Sovereign SDK modules, not Rust modules) have some misplaced unit tests in module/src/tests.rs which should actually be integration tests in module/tests/. We'd like to change this everywhere within the workspace, and expose private items if necessary.

[citizen-stig]

Additional issue that can be good to fix for sequencer:

• sov-sequencer losses transactions on submission error #1090