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.

Sign up for GitHub

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 portal_*Offer jsonrpc call to accept multiple key-value pairs #342

Merged
merged 6 commits into from
Oct 2, 2024
Merged

Update portal_*Offer jsonrpc call to accept multiple key-value pairs #342

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2714aee
Update portal_*Offer jsonrpc call to accept multiple key-value pairs
KolbyML Sep 25, 2024
b85fcc1
update summary
KolbyML Sep 26, 2024
f62f44f
tried to resolve pr concerns
KolbyML Sep 30, 2024
1b10f60
fix ci
KolbyML Sep 30, 2024
56c1757
fix: pr concerns
KolbyML Oct 2, 2024
16439d7
fix: add limit to content items
KolbyML Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
jsonrpc/openrpc.json
jsonrpc/node_modules
11 changes: 11 additions & 0 deletions jsonrpc/src/content/params.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,17 @@
}
}
},
"ContentItems": {
"name": "content_items",
"required": true,
"schema": {
"title": "content_item",
"type": "array",
"items": {
"$ref": "#/components/schemas/ContentItem"
}
}
},
"Enr": {
"name": "enr",
"required": true,
Expand Down
7 changes: 2 additions & 5 deletions jsonrpc/src/methods/beacon.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,16 +99,13 @@
},
{
"name": "portal_beaconOffer",
"summary": "Send an OFFER request with given ContentKey, to the designated peer and wait for a response.",
"summary": "Send an OFFER request with given array of content items (keys & values), to the designated peer and wait for a response. The client MUST return an error if more than 64 content items are provided or less than 1 content items are provided..",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

"params": [
{
"$ref": "#/components/contentDescriptors/Enr"
},
{
"$ref": "#/components/contentDescriptors/ContentKey"
},
{
"$ref": "#/components/contentDescriptors/ContentValue"
"$ref": "#/components/contentDescriptors/ContentItems"
}
],
"result": {
Expand Down
7 changes: 2 additions & 5 deletions jsonrpc/src/methods/history.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,16 +99,13 @@
},
{
"name": "portal_historyOffer",
"summary": "Send an OFFER request with given ContentKey, to the designated peer and wait for a response.",
"summary": "Send an OFFER request with given array of content items (keys & values), to the designated peer and wait for a response. The client MUST return an error if more than 64 content items are provided or less than 1 content items are provided..",
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick there's 2 periods at the end of this sentence

"params": [
{
"$ref": "#/components/contentDescriptors/Enr"
},
{
"$ref": "#/components/contentDescriptors/ContentKey"
},
{
"$ref": "#/components/contentDescriptors/ContentValue"
"$ref": "#/components/contentDescriptors/ContentItems"
}
],
"result": {
Expand Down
7 changes: 2 additions & 5 deletions jsonrpc/src/methods/state.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,16 +99,13 @@
},
{
"name": "portal_stateOffer",
"summary": "Send an OFFER request with given ContentKey, to the designated peer and wait for a response.",
"summary": "Send an OFFER request with given array of content items (keys & values), to the designated peer and wait for a response. The client MUST return an error if more than 64 content items are provided or less than 1 content items are provided..",
"params": [
{
"$ref": "#/components/contentDescriptors/Enr"
},
{
"$ref": "#/components/contentDescriptors/ContentKey"
},
{
"$ref": "#/components/contentDescriptors/ContentValue"
"$ref": "#/components/contentDescriptors/ContentItems"
}
],
"result": {
Expand Down
19 changes: 19 additions & 0 deletions jsonrpc/src/schemas/portal.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,5 +38,24 @@
"title": "UDP port number",
"type": "string",
"pattern": "^([1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5])$"
},
"ContentItem": {
"title": "content_item",
"type": "array",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this an array? Shouldn't it be an object?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ScottyPoi said it should be a tupel? but that doesn't exist so I did an array.

I will let you guys discuss what we should use as I am impartial as long as I can use this RPC command to test the spec. I do think an object would be potentially cleaner though, but I haven't looked too into this yet

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm OK with it either way

Copy link
Member Author

@KolbyML KolbyML Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I implemented it with tuples and it feels good so I am on the tuple side now

Copy link
Collaborator

@kdeme kdeme Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I implemented it with tuples and it feels good so I am on the tuple side now

This statement is rather confusing to me. Because afaik tuples don't exist in JSON, and when they are translated from other languages I think they are typically translated to objects.

An object seems also more natural to me here, I think you don't need that "minItems": 2, "maxItems": 2, in that case.
This will do too, but I wanted to highlight it for others adjusting this rpc, as I was implementing it in fluffy with tuple/object, and then realized its an actually array.

Copy link
Contributor

Choose a reason for hiding this comment

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

and when they are translated from other languages I think they are typically translated to objects.

sorry for the confusion. i thought "tuple" was universally an array-like structure, but with constraints (i.e. length: 2)

"items": [
{
"title": "Content key",
"description": "The encoded Portal content key",
"$ref": "#/components/schemas/hexString"
},
{
"title": "Content value",
"description": "The encoded Portal content value",
"$ref": "#/components/schemas/hexString"
}
],
"minItems": 2,
"maxItems": 2,
"additionalItems": false
}
}
Loading