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

Update EIP-5792: Add status codes #9221

Merged
merged 7 commits into from
Jan 8, 2025

Conversation

lukasrosario
Copy link
Contributor

  • Updated status codes as discussed in telegram chat:
    • 1xx: Pending states
    • 2xx: Confirmed states
    • 4xx: Chain rules failures
    • 5xx: Offchain failures

@lukasrosario lukasrosario requested a review from eth-bot as a code owner January 8, 2025 20:04
@github-actions github-actions bot added c-status Changes a proposal's status t-interface labels Jan 8, 2025
@eth-bot
Copy link
Collaborator

eth-bot commented Jan 8, 2025

✅ All reviewers have approved.

@eth-bot eth-bot changed the title Add status codes Update EIP-5792: Add status codes Jan 8, 2025
@eth-bot eth-bot enabled auto-merge (squash) January 8, 2025 20:05
eth-bot
eth-bot previously approved these changes Jan 8, 2025
Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

auto-merge was automatically disabled January 8, 2025 20:07

Head branch was pushed to by a user without write access

@eth-bot eth-bot enabled auto-merge (squash) January 8, 2025 20:08
Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

calls: {
to?: `0x${string}` | undefined;
data?: `0x${string}` | undefined;
value?: `0x${string}` | undefined; // Hex value
chainId?: `0x${string}` | undefined; // Hex chain id
capabilitiesData?: Record<string, any> | undefined;
Copy link
Contributor

@jxom jxom Jan 8, 2025

Choose a reason for hiding this comment

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

Keep consistent with capabilities?

Suggested change
capabilitiesData?: Record<string, any> | undefined;
capabilities?: Record<string, any> | undefined;

@@ -122,38 +136,59 @@ Returns the status of a call batch that was sent via `wallet_sendCalls`. The ide
type GetCallsParams = string;

type GetCallsResult = {
status: 'PENDING' | 'CONFIRMED';
callsId: `0x${string}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

"calls" seems redundant in this context?

Suggested change
callsId: `0x${string}`;
id: `0x${string}`;

Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

status: 'PENDING' | 'CONFIRMED';
callsId: `0x${string}`;
chainId?: `0x${string}`;
batchStatus: string; // See "Status Codes"
Copy link
Contributor

Choose a reason for hiding this comment

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

"batch" seems redundant in this context?

Suggested change
batchStatus: string; // See "Status Codes"
status: string; // See "Status Codes"

blockHash: `0x${string}`;
blockNumber: `0x${string}`;
gasUsed: `0x${string}`;
transactionHash: `0x${string}`;
}[];
capabilitiesResultsData?: Record<string, any> | undefined;
Copy link
Contributor

@jxom jxom Jan 8, 2025

Choose a reason for hiding this comment

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

"resultsData" seems redundant in this context?

Suggested change
capabilitiesResultsData?: Record<string, any> | undefined;
capabilities?: Record<string, any> | undefined;

Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

@@ -233,15 +268,15 @@ The capabilities below are for illustrative purposes.
{
"0x2105": {
"paymasterService": {
"supported": true
"version": "1.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

How does semver work here? Could be a pain to manage the behavior. Should it be consistent with other version values across other JSON-RPC methods?

Suggested change
"version": "1.0.0"
"version": "1"

Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

@eth-bot eth-bot merged commit 3c4dc08 into ethereum:master Jan 8, 2025
17 of 19 checks passed
Copy link

github-actions bot commented Jan 8, 2025

The commit 0a705ca (as a parent of 8df9866) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jan 8, 2025
Comment on lines +170 to +171
* 4xx: Chain rules failures
* 5xx: Offchain failures
Copy link
Contributor

@jxom jxom Jan 8, 2025

Choose a reason for hiding this comment

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

This is a total nit, but if we are using the analogy of HTTP status codes here, shouldn't it be the other way round?

4xx: HTTP Client Error ≈ Offchain Error
5xx: HTTP Server Error ≈ Onchain/chain rules Error

I would see "offchain" similarly to a HTTP Client, and "onchain"/chain rules similarly to a HTTP Server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-status Changes a proposal's status t-interface w-ci Waiting on CI to pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants