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

archive/call: Synchronize the return type with sudo/sessionsKeys #104

Merged
merged 5 commits into from
Feb 26, 2024

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Sep 22, 2023

This PR simplifies the return type of the archive_unstable_call method.

This effectively changes the return object from success: true, value: ... to result:... and success: false, error:... to error:... to keep consistency with the sudo_sessionsKeys format.

The notes have been adjusted to reflect feedback from: #94.

@paritytech/subxt-team @tomaka @josepot @jsdw

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv self-assigned this Sep 22, 2023
@tomaka
Copy link
Contributor

tomaka commented Sep 22, 2023

Throughout the entire API, JSON-RPC errors are only ever used in situations where the API is misused. This is intentional so that JSON-RPC client developers don't need to worry about these errors.

@lexnv
Copy link
Contributor Author

lexnv commented Sep 22, 2023

Just to be sure I got this right:
The result of the archive_call method is returned similar to:

// Ok result wrapped in JSON-RPC Ok response
{
    "jsonrpc": "2.0",
    "id": 1, 
    "result": { "success": true, "value": ... },
}

// Error result wrapped in JSON-RPC Ok response
{
    "jsonrpc": "2.0",
    "id": 1, 
    "result": { "success": false, "error": ... },
}

And we are only returning a JSON-RPC error object {"jsonrpc": "2.0", "error": {"code": -32601, "message": ".."}, "id": "1"} for the possible error section?

@josepot
Copy link
Contributor

josepot commented Sep 23, 2023

I'm not entirely certain if this is the appropriate forum for this suggestion, but I believe it would be beneficial if archive methods could accept either a hash or a block-number. My rationale for this is:

  1. Binary searches are frequently employed when interacting with archive nodes. By allowing for block-number input, these searches could be facilitated and made more efficient.

  2. While I don't claim expertise in this domain, it appears logical to me that archive nodes would have block numbers thoroughly indexed. If this assumption holds, then accessing data using a block number should be just as quick and efficient as using a block hash.

I hope my intuition aligns with the technical realities, and I'd appreciate any insight or clarification on this matter.

@tomaka
Copy link
Contributor

tomaka commented Sep 23, 2023

Just to be sure I got this right: The result of the archive_call method is returned similar to:

// Ok result wrapped in JSON-RPC Ok response
{
    "jsonrpc": "2.0",
    "id": 1, 
    "result": { "success": true, "value": ... },
}

// Error result wrapped in JSON-RPC Ok response
{
    "jsonrpc": "2.0",
    "id": 1, 
    "result": { "success": false, "error": ... },
}

And we are only returning a JSON-RPC error object {"jsonrpc": "2.0", "error": {"code": -32601, "message": ".."}, "id": "1"} for the possible error section?

Yes

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
Copy link
Contributor Author

lexnv commented Sep 25, 2023

Have modified the resulting object to align with sudo_sessionKeys, such that we could reuse it across the implementation methods.

Let me know what you think of it since it removes the success flag 🙏

// Error
{
    "jsonrpc": "2.0",
    "id": 1, 
    "result": { "error": ... },
}

// Ok
{
    "jsonrpc": "2.0",
    "id": 1, 
    "result": { "result": ... },
}

Introducing this object seems oddly similar to a json-rpc object (https://github.com/paritytech/polkadot-sdk/pull/1682/files#r1335584309). Was this extension also motivated by json-rpc clients dropping / ignoring the error component? 🤔

@lexnv lexnv changed the title archive/call: Return a JSON-RPC object archive/call: Synchronize the return type with sudo/sessionsKeys Sep 26, 2023
jsdw
jsdw previously approved these changes Jan 23, 2024
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

LGTM with the rationale that it makes the method more consistent with https://paritytech.github.io/json-rpc-interface-spec/api/sudo_sessionKeys_unstable_generate.html

(if there was a desire to go the other way and add success to both I'd also be happy).

@tomaka any remaining objections to merging this?

@tomaka
Copy link
Contributor

tomaka commented Jan 23, 2024

Let me know what you think of it since it removes the success flag

I included this success more or less by reflex, because it's easier to write TypeScript definitions with such a flag that unambiguously distinguishes between situations.
But I don't care either way.

@jsdw
Copy link
Collaborator

jsdw commented Jan 23, 2024

I included this success more or less by reflex, because it's easier to write TypeScript definitions with such a flag that unambiguously distinguishes between situations.

I'm sortof the same; I don't care strongly either way but happy for a touch of extra consistency across the spec

@jsdw
Copy link
Collaborator

jsdw commented Feb 21, 2024

@lexnv perhaps we should just keep the success bool for both cases since it does help writing TS types a little, and then this is good to merge I think!

jsdw
jsdw previously approved these changes Feb 26, 2024
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Works for me!

Co-authored-by: James Wilson <james@jsdw.me>
@lexnv lexnv merged commit 5c2a542 into main Feb 26, 2024
3 checks passed
@lexnv lexnv deleted the lexnv/archive_call branch February 26, 2024 13:42
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.

4 participants