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

Jump to bottom

add ws socket address in config and e2e test for event #2597

Merged
merged 2 commits into from
Jun 22, 2022
Merged

add ws socket address in config and e2e test for event #2597

merged 2 commits into from
Jun 22, 2022

Conversation

longbowlu
Copy link
Contributor

as title

@longbowlu longbowlu requested review from velvia and patrickkuo June 16, 2022 17:58
@longbowlu longbowlu changed the title add ws socket address in config and e2e test add ws socket address in config and e2e test for event Jun 16, 2022
@longbowlu longbowlu requested review from bmwill and mystenmark June 16, 2022 20:31
patrickkuo
patrickkuo approved these changes Jun 17, 2022
View reviewed changes
Copy link
Contributor

@patrickkuo patrickkuo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the test

crates/sui/tests/full_node_tests.rs Outdated Show resolved Hide resolved
@bmwill
Copy link
Contributor

bmwill commented Jun 17, 2022

Hold on, we should not need yet another address for doing websocket stuff

bmwill
bmwill previously requested changes Jun 17, 2022
View reviewed changes
Copy link
Contributor

@bmwill bmwill left a comment

Choose a reason for hiding this comment

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

Let's please not add another network interface

velvia
velvia approved these changes Jun 18, 2022
View reviewed changes
Copy link
Contributor

@velvia velvia left a comment

Choose a reason for hiding this comment

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

Great to have an end to end test.

Maybe, try emitting more than one event, or just run that NFT thing twice?

@bmwill bmwill dismissed their stale review June 19, 2022 20:39

While I do not like that we're needing to add another networking interface for something that can be trivially done via other web frameworks, i'll remove my blocking review with the understanding that we should fix this at some point

@longbowlu longbowlu force-pushed the port-in-config-and-e2e-test branch from 53cc57a to 427b9d5 Compare June 22, 2022 07:15
@longbowlu longbowlu enabled auto-merge (squash) June 22, 2022 07:15
@longbowlu longbowlu merged commit 76eb5aa into main Jun 22, 2022
@longbowlu longbowlu deleted the port-in-config-and-e2e-test branch June 22, 2022 07:25
punwai pushed a commit that referenced this pull request Jun 30, 2022
@longbowlu
add ws socket address in config and e2e test for event (#2597)
94ad1a0 
* add ws socket in config and e2e test

* rebase & comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mystenmark mystenmark mystenmark left review comments

@patrickkuo patrickkuo patrickkuo approved these changes

@velvia velvia velvia approved these changes

@bmwill bmwill bmwill left review comments

Assignees
No one assigned
Labels
event
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@longbowlu @bmwill @velvia @patrickkuo @mystenmark