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

refactor(rpc-alt): pass configs via Context #21332

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

amnn
Copy link
Contributor

@amnn amnn commented Feb 24, 2025

Description

A couple of refactorings, mostly to do with configs:

  • The main change is that instead of breaking apart the config and giving it to each module that needed it, it is now stored in the Context where all modules can access it. The motivation for this came from working on showDisplay which is relevant to a multiple modules but is quite deeply nested, so threading the config into the right place became quite tedious/noisy.
  • As part of this change, I split apart the old RpcConfig into an RpcLayer and RpcConfig -- this was so that the Context could hold on to one value (of RpcConfig) rather than having to pass each module's config to it individually (and access them individually through their own modifiers).
  • This PR also cleans up various imports across the rpc-alt codebase, to follow a standard order and style (not a big deal, but seeing as I noticed, I thought I would clean it up): imports are grouped by their top-level identifier, and we order imports as follows (with a blank line between each group): std::, dependency imports, crate::, super::, self::.

Test plan

CI


Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

  • Protocol:
  • Nodes (Validators and Full nodes):
  • gRPC:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:

amnn added 3 commits February 22, 2025 17:41
## Description
Small tweak on config files -- it's already in the config so we don't
need to add the suffix here.

## Test plan
CI
## Description

Gather all the configs together and expose them through the `Context`,
rather than handing out the configs needed to each module as a field.

The motivation for this came from working on Display -- part of handling
object responses, which shows up across multiple RPC modules (objects,
query objects, dynamic fields), but deeply nested in the call stack and
requires some configuration.

With the old method of propagating configs, we would need to make the
display config available to all the modules that handle object responses
and thread it through the function call stack, even if e.g. dynamic
fields use object responses but never ask for Display.

With this new system, we piggy-back on the fact that the `Context`
parameter is already threaded into all these functions.

## Test plan

```
sui$ cargo nextest run         \
  -p sui-indexer-alt-jsonrpc   \
  -p sui-indexer-alt-e2e-tests
```
## Description
Standardise the order of imports, to help `rust-analyzer` maintain it
consistently in future:

- Order imports as follows: `std`, dependencies, `crate`, `super`,
  `self`.
- Group together the `crate` and `super` imports at the top-level.

## Test plan
CI
@amnn amnn requested review from emmazzz, gegaowp and wlmyng February 24, 2025 13:12
@amnn amnn self-assigned this Feb 24, 2025
Copy link

vercel bot commented Feb 24, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Feb 24, 2025 1:12pm
sui-kiosk ⬜️ Ignored (Inspect) Feb 24, 2025 1:12pm

@amnn amnn temporarily deployed to sui-typescript-aws-kms-test-env February 24, 2025 13:12 — with GitHub Actions Inactive
@amnn amnn mentioned this pull request Feb 24, 2025
7 tasks
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.

1 participant