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

Fix ApiHeadChange type #4357

Merged
merged 9 commits into from
Jun 5, 2024
Merged

Fix ApiHeadChange type #4357

merged 9 commits into from
Jun 5, 2024

Conversation

elmattic
Copy link
Contributor

@elmattic elmattic commented May 22, 2024

Summary of changes

Changes introduced in this pull request:

  • Fix ApiHeadChange type

Reference issue to close (if applicable)

Closes #4371, #4002

Other information and links

The endpoint code is only being manually tested at the moment. I've attached the script used and what the result files look like once you have run it (using node index.js).
index.txt
forest.log
lotus.log

In the attached log files, we should have an exact match for notifications of the type "current". We should have a match for notifications of type "apply" when Val.Cids values are the same.

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

@elmattic elmattic marked this pull request as ready for review May 22, 2024 18:16
@elmattic elmattic requested a review from a team as a code owner May 22, 2024 18:16
@elmattic elmattic requested review from ruseinov and lemmih and removed request for a team May 22, 2024 18:16
@LesnyRumcajs
Copy link
Member

@alt-ivan could you please confirm that this change works for you?

@alt-ivan
Copy link

@LesnyRumcajs
Sorry, I haven't been able to test yet due to other obligations. I will test on Monday and report back here.

@alt-ivan
Copy link

alt-ivan commented May 28, 2024

@LesnyRumcajs
I have finally got around to testing this and the type change introduced in this PR works.
However I found some other ChainNotify issues, opened an issue here: #4371

@elmattic
Copy link
Contributor Author

@alt-ivan Many thanks for reporting this issue. I've committed a patch in this PR that should fix #4371.

Let me know if everything works well for you.

@alt-ivan
Copy link

@elmattic
You are welcome! I re-tested and left a comment on the issue

@elmattic elmattic marked this pull request as draft May 28, 2024 16:00
@elmattic
Copy link
Contributor Author

elmattic commented May 29, 2024

@alt-ivan I've committed a fix to properly support several WS clients. Can you have a second look? I've tested the following scenario on my side (on both macOS and Ubuntu):

  • Two WS clients connect to the Forest node, where the second client creates two channels.

All three channel outputs look alike.

Channel cancellation works properly.

@alt-ivan
Copy link

@elmattic

Awesome, thanks! I'll report back here as soon as I re-test everything

@alt-ivan
Copy link

alt-ivan commented Jun 5, 2024

@elmattic

Hi, I have re-tested and can confirm that everything works as expected. I apologize for the delay!

@elmattic
Copy link
Contributor Author

elmattic commented Jun 5, 2024

Hi, I have re-tested and can confirm that everything works as expected. I apologize for the delay!

No worries! Thanks for your help with the testing.

@elmattic elmattic marked this pull request as ready for review June 5, 2024 11:25
@elmattic elmattic added this pull request to the merge queue Jun 5, 2024
@elmattic elmattic mentioned this pull request Jun 5, 2024
4 tasks
Merged via the queue into main with commit 95bca40 Jun 5, 2024
27 checks passed
@elmattic elmattic deleted the elmattic/fix-api-head-change-wo-test branch June 5, 2024 13:48
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.

ChainNotify erroneous behavior
6 participants