-
Notifications
You must be signed in to change notification settings - Fork 29
feat: add trace-offer #795
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
base: master
Are you sure you want to change the base?
Conversation
ScottyPoi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
We have not been developing on Ultralight recently, as we evaluate the future of Portal Network in general. Reviewing this with some optimism that some version of Portal will still be used by EL clients.
Good work overall. I left a few comments to address. There's a bug in how the new RPC methods are dealing with the response. I'll take some blame there, as the upgrade to protocol V1 was not well documented in the code. The upgrade from v0 to v1 involved changing the return type of the contentKeys array in an ACCEPT response. With v0 we used a BitArray, and with V1 it is a Unit8Array. Currently things are set up to support both versions, so we'll need to handle the reponse differently if it's a BitArray or Uint8Array for now, and you won't want to call your helper function on a Uint8Array response.
| import type { BeaconNetwork, HistoryNetwork, PortalNetwork, StateNetwork } from 'portalnetwork' | ||
| import type { GetEnrResult } from '../schema/types.js' | ||
|
|
||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move this to rpc/utils.ts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks that's the info I was looking for 🙏
|
|
||
| // Should return success with boolean array indicating which keys were accepted | ||
| assert.ok(res.result.success !== undefined, 'should have success array') | ||
| if (res.result.success === true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res.result.success is a boolean array, so this part gets skipped
you've already asserted that it is not undefined, so you can just remove this conditional and let the other checks happen
| } | ||
|
|
||
| const res = await this._history.sendOffer(enr, contentKeys, contentValues) | ||
| return bitToBooleanArray(res, contentKeys.length) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The response will only be a BitArray if running protocol V0
With V1 this type was changed to Uint8Array. The current state of Ultralight is still designed to support both.
But if the response is V1 you won't want to call this helper function, which would actually end up returning a full array of accepts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, let me see and update accordingly to support both
| } | ||
|
|
||
| const res = await this._state.sendOffer(enr, contentKeys, contentValues) | ||
| return bitToBooleanArray(res, contentKeys.length) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here as history method
| } | ||
|
|
||
| const res = await this._beacon.sendOffer(enr, contentKeys, contentValues) | ||
| return bitToBooleanArray(res, contentKeys.length) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here as history method
change any to BitArray type Co-authored-by: Scotty <[email protected]>
…ralight into feat-add-trace-offer
|
@ScottyPoi thanks for the review:
Can you confirm what should be return in case we don't have Also I should not continue to take other issue if project is on halt? Where can I follow the state of decision for the future of Portal Network? |
| // Store content in node2 | ||
| await rp2.request('portal_historyStore', [contentKey1, content1]) | ||
| await rp2.request('portal_historyStore', [contentKey2, content2]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If node2 stores these contents first, it will reject them in the OFFER. This test is giving you a false positive because of the bug in the Uint8ToBooleanArray helper
The original set of Portal Networks are now "Legacy" and no longer live. The only Portal Network currently in development is a new, minimal version of history. I'm going to rename all of the old networks with a The work you're doing now is still helpful though! I plan to implement the new |
check only AccetCode in UInt8toBoolenArray Co-authored-by: Scotty <[email protected]>
Implement traceOffer function.
It should return an object with this form:
Function have exactly the same behavior as offer, the difference is on what it returns.
A helper function to convert bit to boolean array has been added to improve reusability. For the moment it's in
portal.tsmaybe there is a better place to put this helper?Test case has been added where we try 3 cases:
This solve #729