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

Conflict between empty receipts and unsuccessful RFC lookups #256

Open
njgheorghita opened this issue Jan 11, 2024 · 5 comments
Open

Conflict between empty receipts and unsuccessful RFC lookups #256

njgheorghita opened this issue Jan 11, 2024 · 5 comments

Comments

@njgheorghita
Copy link
Contributor

In the current jsonrpc spec, for portal_*recursiveFindContent & portal_*TraceRecursiveFindContent, if the lookup is not able to find the target information, a return value of "0x" is expected (If the content is not available, returns "0x"). However, "0x" is also a valid value for an empty, ssz-encoded Receipts type (reference).

Therefore, it is impossible for a user to distinguish between a failed lookup for a Receipts content value, or a successful lookup that returns an empty Receipts type. I also expect that there might be future content values where we want "0x" to be a valid value.

It seems unavoidable that we need to change the If the content is not available, return "0x" convention for RFC lookups.

Some possible solutions...

  1. If the content is not available, return "null"
  • null is a valid json type, so if the RFC lookup fails, then simply return null as the content value.

RFC example

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": {
    "content": null,
    "utpTransfer": false
  }
}

TRFC example

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": {
    "content": null,
    "utpTransfer": false,
    "trace": {...}
  }
}
  1. Return a json error if the RFC lookup fails.

RFC example

{
  "jsonrpc": "2.0",
  "id": 1,
  "error": {
    "code": 32000,
    "message": "Unable to find content in RFC lookup."
  }
}

TRFC example

{
  "jsonrpc": "2.0",
  "id": 1,
  "error": {
    "code": 32000,
    "message": "Unable to find content in RFC lookup.",
    "data": {..trace_info..}
  }
}

Personally, I'm in favor of solution 1, because it's simpler. I'm not convinced that we should treat an "unsuccessful" RFC lookup as an error. But, I don't have any strong objections if that's the consensus. If there are any other possible solutions / thoughts / feedback, please leave them in a comment below.

@kdeme
Copy link
Collaborator

kdeme commented Jan 12, 2024

Personally, I'm in favor of solution 1, because it's simpler. I'm not convinced that we should treat an "unsuccessful" RFC lookup as an error. But, I don't have any strong objections if that's the consensus.

I'm of the same opinion, but also not strongly against the the other solution.

@ogenev
Copy link
Member

ogenev commented Jan 16, 2024

I'm also in favor of option 1).

@pipermerriam
Copy link
Member

I believe option 1 is probably a "foot gun" and that it's likely for implementations to get this wrong or for users to interpret it wrong...

Which suggest that option 2 is likely to be less likely to result in problems...

This isn't a strong opinion and you guys can go whichever way you like.

@morph-dev
Copy link
Contributor

There is also option 3, that is similar to option 1 but has explicitness of option 2.

  1. Use Union type from SSZ specification. In practice, this would add 1 extra byte (e.g. 0 for absent, 1 for present), which would result in something like this:

RFC example - content not available

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": {
    "content": "0x00",
    "utpTransfer": false
  }
}

content is empty list

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": {
    "content": "0x01",
    "utpTransfer": false
  }
}

content is 0xabc...

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": {
    "content": "0x01abc",
    "utpTransfer": false
  }
}

This also wouldn't need special (de)serialization as this is standard type.

@njgheorghita
Copy link
Contributor Author

Personally, I don't love the idea of wrapping our responses in another level of ssz-encoding, it could just get confusing especially when none of our other endpoints behave this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants