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

chore: update example #291

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

chore: update example #291

wants to merge 2 commits into from

Conversation

jeluard
Copy link

@jeluard jeluard commented Jun 3, 2024

Make sure the libp2p integration example reflects the latest libp2p version.

@jeluard jeluard requested a review from a team as a code owner June 3, 2024 20:31
import PeerId from "peer-id";

const myPeerId: PeerId = ...;

const bootstrapEnrs: ENR[] = [...];
const bootEnrs: ENR[] = [...];
Copy link
Member

Choose a reason for hiding this comment

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

can you add an import for this type?

Copy link
Member

@nflaig nflaig Jun 3, 2024

Choose a reason for hiding this comment

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

we have a general issue with our readme examples also in the Lodestar repo, might wanna spend some time investigating ChainSafe/lodestar#6300. This would ensure the examples are runnable and detect issues due to code refactoring / dependency updates.

Copy link
Author

Choose a reason for hiding this comment

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

@wemeetagain done!

@jeluard jeluard requested a review from wemeetagain June 4, 2024 08:33
import { Discv5Discovery, ENR } from "@chainsafe/discv5";
import Libp2p from "libp2p";
import {Discv5Discovery} from "@chainsafe/discv5";
import {ENR, SignableENR} from "@chainsafe/enr";
Copy link
Member

@nflaig nflaig Jun 4, 2024

Choose a reason for hiding this comment

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

do we have to import these from enr package or does discv5 re-export? the later seems better devex to me

Copy link
Author

Choose a reason for hiding this comment

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

I haven't seen them being re-exported. Maybe @wemeetagain knows better?

@jeluard jeluard requested a review from nflaig June 11, 2024 16:14
Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

I don't like that we import stuff from enr package, other than that seems fine to me

@nflaig
Copy link
Member

nflaig commented Jun 11, 2024

Two follow-ups to this PR

  • re-export enr from discv5
  • add docs verifier to monorepo

Also @wemeetagain should probably give this a final ✔️

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