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

feat: add feature RPC #2719

Merged
merged 13 commits into from
Jul 10, 2024
Merged

feat: add feature RPC #2719

merged 13 commits into from
Jul 10, 2024

Conversation

mvadari
Copy link
Collaborator

@mvadari mvadari commented Jul 1, 2024

High Level Overview of Change

This PR adds the feature RPC to the models.

Context of Change

There is now a non-admin user version of this RPC: XRPLF/rippled#4781

Type of Change

  • New feature (non-breaking change which adds functionality)

Did you update HISTORY.md?

  • Yes

Test Plan

Added integration tests.

@mvadari mvadari marked this pull request as draft July 1, 2024 13:51
@mvadari mvadari marked this pull request as ready for review July 2, 2024 18:39
@mvadari mvadari requested a review from ckeshava July 5, 2024 20:20
export interface FeatureAllRequest extends BaseRequest {
command: 'feature'

feature?: never
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
feature?: never

Can we remove this line?
I've seen never used in the context of infeasible control flows. Why do we need to state the absence of a field in an interface?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's necessary to distinguish the types. One weirdness about TS types is that you can add on extra fields and it'll still be that type. So this is still a valid FeatureAllRequest object according to TS without this field:

{
  command: 'feature'
  feature: 'something'
}

Adding the never is needed to ensure that this isn't actually a valid FeatureAllRequest type.

The ? is needed because for some reason you get errors for 'feature' field not included in the FeatureAllRequest type if you don't have it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @justinr1234, our local expert on TS types

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will look

Copy link
Collaborator Author

@mvadari mvadari Jul 8, 2024

Choose a reason for hiding this comment

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

An example in our codebase:

export interface MissingPreviousTxnID {
/**
* This field is missing on this object but is present on most other returned objects.
*/
PreviousTxnID: never
/**
* This field is missing on this object but is present on most other returned objects.
*/
PreviousTxnLgrSeq: never
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, I did come across other examples in the codebase

Copy link
Collaborator

Choose a reason for hiding this comment

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

I only saw control-flow related examples in the Typescript docs website: https://www.typescriptlang.org/docs/handbook/2/functions.html#never

It just feels strange that we are listing non-existent variables in the interface. But I get the rationale as a work-around to typescript's type inference.

This is not a strong objection, but it's nice to not have to do this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that it's kind of ugly, but I think it's necessary in TypeScript's typing system.

@mvadari mvadari requested a review from ckeshava July 8, 2024 22:15
@mvadari mvadari merged commit 00f1a6b into main Jul 10, 2024
15 checks passed
@mvadari mvadari deleted the feature-rpc branch July 10, 2024 17:18
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