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

rpc-alt: redefine TransactionFilter #21203

Merged
merged 0 commits into from
Feb 16, 2025
Merged

Conversation

amnn
Copy link
Contributor

@amnn amnn commented Feb 13, 2025

Description

Rather than re-using the base JSON-RPC TransactionFilter type and having to respond that certain filters are unsupported, define our own compatible type which only contains the variants we do support. This way we will still return an invalid params error if someone tries to use an unsupported filter, but the filters we advertise in the schema will match what we can return a non-error response for.

Test plan

Existing tests.

Stack


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 amnn requested review from emmazzz, gegaowp and wlmyng February 13, 2025 14:53
@amnn amnn self-assigned this Feb 13, 2025
Copy link

vercel bot commented Feb 13, 2025

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

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 15, 2025 11:06pm
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Feb 15, 2025 11:06pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Feb 15, 2025 11:06pm

Copy link
Contributor

@emmazzz emmazzz left a comment

Choose a reason for hiding this comment

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

Nice change!


#[serde_as]
#[derive(Clone, Debug, JsonSchema, Serialize, Deserialize)]
pub(crate) enum TransactionFilter {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the error that gets returned now if a user uses one of the unsupported filters from the OG filter enum? I suppose it will be rejected at the beginning (before even entering our code) with invalid params and a message like "wrong variant of enum"? Would like to make sure the message is clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, something like that -- here's an example:

{
  "jsonrpc": "2.0",
  "id": 1,
  "error": {
    "code": -32602,
    "message": "Invalid params",
    "data": "unknown variant `ToAddress`, expected one of `Checkpoint`, `MoveFunction`, `AffectedObject`, `FromAddress`, `FromAndToAddress`, `FromOrToAddress` at line 1 column 25"
  }
}

Copy link
Contributor

@gegaowp gegaowp left a comment

Choose a reason for hiding this comment

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

lgtm

@amnn amnn force-pushed the amnn/rpc-tx-prune-test branch from 66a1c18 to 242e956 Compare February 15, 2025 22:59
@amnn amnn force-pushed the amnn/rpc-tx-filter branch from 743b1a0 to b2b6111 Compare February 15, 2025 22:59
@amnn amnn temporarily deployed to sui-typescript-aws-kms-test-env February 15, 2025 22:59 — with GitHub Actions Inactive
@amnn amnn force-pushed the amnn/rpc-tx-filter branch from b2b6111 to eb6d686 Compare February 16, 2025 01:35
@amnn amnn merged commit eb6d686 into amnn/rpc-tx-prune-test Feb 16, 2025
@amnn amnn force-pushed the amnn/rpc-tx-prune-test branch from 242e956 to eb6d686 Compare February 16, 2025 01:35
@amnn amnn deleted the amnn/rpc-tx-filter branch February 16, 2025 01:35
@amnn amnn temporarily deployed to sui-typescript-aws-kms-test-env February 16, 2025 01:35 — with GitHub Actions Inactive
@amnn amnn temporarily deployed to sui-typescript-aws-kms-test-env February 16, 2025 01:35 — with GitHub Actions Inactive
@amnn amnn temporarily deployed to sui-typescript-aws-kms-test-env February 16, 2025 01:35 — with GitHub Actions Inactive
@amnn amnn temporarily deployed to sui-typescript-aws-kms-test-env February 16, 2025 01:35 — with GitHub Actions Inactive
@amnn amnn temporarily deployed to sui-typescript-aws-kms-test-env February 16, 2025 01:35 — with GitHub Actions Inactive
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