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

traceGetContent spec questions #352

Open
acolytec3 opened this issue Nov 6, 2024 · 2 comments
Open

traceGetContent spec questions #352

acolytec3 opened this issue Nov 6, 2024 · 2 comments

Comments

@acolytec3
Copy link
Contributor

acolytec3 commented Nov 6, 2024

While implementing the *traceGetContent endpoints in ultralight, I discovered that there are a couple of places in the traceResultObject where what trin returns doesn't match the current spec but that seem to be required for glados to support tracing of audits.

  • targetId - the spec calls for ^0x([1-9a-f][0-9a-f]{0,63}|0$) but trin returns an array of integers (presumably just converting byte array representation of the contentId directly to a JSON array).
    • @KolbyML confirmed this would be updated so trin and glados would accept the 0x prefixed string version of the contentID.
  • startedAtMs - the spec calls for an integer but trin returns an object of the form
    • I'm not certain how the nanos_since_epoch is calculated here since it seems to be a rust struct (from looking at the glados errors I was getting when I didn't populate this field in the Ultralight response) but assuming it makes sense to update the spec to match this (with clear definition of what the two properties are and how to compute them)
"startedAtMs": {
    "secs_since_epoch": 1730857625,
    "nanos_since_epoch": 447534202
}
  • metadata - the spec calls for a map that looks like below. Trin adds a radius field to that always seems to be populated with null (not sure if this is needed for glados or not)
"0xdeadbeef..." : {
    "enr": "enr-...",
    "distance": "0x1234..."
}
  • Since the radius always seems to be null, I'm not sure if this is a helpful additional element or not (though notionally get why it would be useful). Also, not sure if glados needs this value or not for audits

One other thing I noticed is that the playground representation of the spec doesn't currently expose all of the lower level traceResult types (notably traceResultMetadata and traceResultResponses) and you have to actually open the raw spec to see them. We should fix this so they are clickable from the playground interface. I'll figure out how to make these clickable so we can see them.

@KolbyML
Copy link
Member

KolbyML commented Nov 27, 2024

I made PR's for

For the metadata comment

it was added to Trin in ethereum/trin#1332 I will let @mrferris decide on how we will fix that.

@kdeme
Copy link
Collaborator

kdeme commented Nov 28, 2024

I verified this in the fluffy implementation, we already have the hexstring for targetId, and 1 value for startedAtMs. We don't pass along the radius in metadata, that could be a feature that we add in additional PR.

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

No branches or pull requests

3 participants