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

feat!: configurable transport service and remote enr #169

Merged
merged 4 commits into from
Apr 26, 2022

Conversation

acolytec3
Copy link
Contributor

Adds the following:

  • Adds a transport option to Discv5.create to all an alternative transport layer to be used by the SessionService provided it implements the ITransportLayer interface and falls back to the native UDP implementation
  • Adds a new optional remoteEnr parameter to the sendTalkReq/sendTalkResp to allow an ENR to be passed into these two message handlers since Portal Network maintains its own routing tables outside of the base discv5 routing table and our tables may contain ENRs that are not found in the base discv5 table.
  • Adds a hack enableLogs to allow the debug logs to be displayed in the browser. I'm open to alternative ways of handling this one but not sure what the options are. I believe this actual suggestion came from @wemeetagain. If you don't like this method in the interface, presumably I should be able to somehow override the default logger from within our portalnetwork module so we can still surface the discv5 logs at the Portal Network level.

I've tested all this locally with our portal network module and it seems to work well.

@acolytec3 acolytec3 requested a review from a team as a code owner April 18, 2022 20:56
@mpetrunic mpetrunic changed the title Service changes to support Portal Network feat: configurable transport service Apr 19, 2022
@mpetrunic mpetrunic changed the title feat: configurable transport service feat: configurable transport service and remote enr Apr 19, 2022
Copy link
Member

@mpetrunic mpetrunic left a comment

Choose a reason for hiding this comment

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

LGTM, @wemeetagain when merging, change the commit message because for "one commit PR" GitHub will prefer commit name rather then PR title (that's why semantic CI is failing)

/**
* Send TALKREQ message to dstId and returns response
*/
public async sendTalkReq(dstId: string, payload: Buffer, protocol: string | Uint8Array): Promise<Buffer> {
public async sendTalkReq(
Copy link
Member

Choose a reason for hiding this comment

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

I think we can change the interface here to be more aligned with what we need to send an Rpc request/response. (Also replace Buffer with Uint8Array.) I'm thinking something like this:

public async sendTalkReq(dst: ENR | Multiaddr, payload: Uint8Array, protocol: string | Uint8Array): Promise<Uint8Array> {
  return await new Promise((resolve, reject) => {
    this.sendRpcRequest({
      contact: createNodeContact(dst),
      request: createTalkRequestMessage(payload, protocol),
      callback: (err: RequestErrorType | null, res: Uint8Array | null): void => {
        if (err !== null) {
          reject(err);
          return;
        }
        resolve(res as Uint8Array);
      },
    });
  });
}

We can also expose findEnr as a public method, so anyone who wants something like the existing situation can find the enr themselves.

const enr = discv5.findEnr(dstId);
if (!enr) {
  throw ...;
}
const resp = await discv5.sendTalkReq(enr, payload, protocol);

And then we can have a similar interface/implementation for sendTalkResp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So make the consuming app provide the ENR up front and remove the ENR lookup entirely from within sendTalkReq (and friends)?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah!

Copy link
Member

Choose a reason for hiding this comment

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

isn't that gonna be breaking change?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it is. If we want the other stuff in the current version, we can break this up into another PR.
Afaik, lodestar and ultralight are the only users of this library, moving quickly to another version shouldn't be an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed requested changes. I wasn't sure the best way to handle the sendTalkResp changes so take a peek and let me know what you think.

acolytec3 and others added 2 commits April 21, 2022 09:55
BREAKING CHANGE: sendTalkReq and sendTalkResp now require an ENR rather
than a NodeId. The findENR method is now exposed to retrieve any locally
cached ENRs for this purpose.
@wemeetagain wemeetagain changed the title feat: configurable transport service and remote enr feat!: configurable transport service and remote enr Apr 26, 2022
@wemeetagain wemeetagain merged commit 72aaa0b into ChainSafe:master Apr 26, 2022
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

Successfully merging this pull request may close these issues.

3 participants