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(wakunode): Resultify fetch peer exchange peers #2486

Merged

Conversation

AlejandroCabeza
Copy link
Contributor

@AlejandroCabeza AlejandroCabeza commented Feb 28, 2024

Description

Upgrade return type of Wakunode.fetchPeerExchangePeers.
Feel free to suggest better error returns.

Changes

  • Change return type from Future to Future[Result[int, string]].

@SionoiS
Copy link
Contributor

SionoiS commented Feb 28, 2024

@AlejandroCabeza can we try again without the formatting changes plz?

@AlejandroCabeza
Copy link
Contributor Author

@AlejandroCabeza can we try again without the formatting changes plz?

Ah, dammit, didn't realise 🤦‍♀️ Will do.

@AlejandroCabeza AlejandroCabeza self-assigned this Feb 29, 2024
@AlejandroCabeza AlejandroCabeza force-pushed the resultify-fetch-peer-exchange-peers branch from ad1418f to 3596843 Compare February 29, 2024 11:45
Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

Thanks! More Result the better

Copy link

github-actions bot commented Feb 29, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2486-rln-v2-false

Built from 78d147f

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

Thanks! It looks great indeed! Just a tiny detail that I consider interesting.

apps/wakunode2/app.nim Outdated Show resolved Hide resolved
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Thanks so much!

tests/node/test_wakunode_peer_exchange.nim Outdated Show resolved Hide resolved
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks so much!

@Ivansete-status
Copy link
Collaborator

Hey @AlejandroCabeza !
Sorry, we can't merge yet until CI are green. We can accept the js-waku test errors because those happen on other PRs.

@AlejandroCabeza
Copy link
Contributor Author

Hey @AlejandroCabeza ! Sorry, we can't merge yet until CI are green. We can accept the js-waku test errors because those happen on other PRs.

Indeed, indeed. I'll have a look what's going on.

@AlejandroCabeza AlejandroCabeza force-pushed the resultify-fetch-peer-exchange-peers branch from 88e7617 to c363431 Compare March 12, 2024 20:52
@AlejandroCabeza AlejandroCabeza merged commit 761a297 into test-peer-exchange Mar 13, 2024
14 of 15 checks passed
@AlejandroCabeza AlejandroCabeza deleted the resultify-fetch-peer-exchange-peers branch March 13, 2024 13:53
AlejandroCabeza added a commit that referenced this pull request Mar 13, 2024
* Resultify Wakunode.fetchPeerExchangePeers
AlejandroCabeza added a commit that referenced this pull request Mar 14, 2024
* Implement peer exchange tests.
* Refactor, and remove duplicated tests.
* feat(wakunode): Resultify fetch peer exchange peers (#2486)
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.

None yet

4 participants