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

Push notification shows wrong deposit amount #763

Closed
ekzyis opened this issue Jan 19, 2024 · 6 comments · Fixed by #940
Closed

Push notification shows wrong deposit amount #763

ekzyis opened this issue Jan 19, 2024 · 6 comments · Fixed by #940
Assignees
Labels

Comments

@ekzyis
Copy link
Member

ekzyis commented Jan 19, 2024

Description

Multiple push messages are sent on deposits. They then get merged to show a wrong amount. See attached video.

Steps to Reproduce

Deposit funds

Expected behavior

Push notifications shows correct amount

Video

2024-01-19.04-54-47.mp4

Additional context

Probably related to #726

@ekzyis
Copy link
Member Author

ekzyis commented Jan 19, 2024

What I found out yesterday: It's related to reconnecting but I verified that the finally branch does execute. I thought it might not because we rethrow the error in catch but it does.

But seems like old subscriptions still continue to fire on reconnection.

@ekzyis
Copy link
Member Author

ekzyis commented Jan 20, 2024

Mhh, can't reproduce locally anymore

@huumn
Copy link
Member

huumn commented Jan 21, 2024

Wasn't this caused by what you fixed in #764?

@ekzyis
Copy link
Member Author

ekzyis commented Jan 21, 2024

Wasn't this caused by what you fixed in #764?

No, #764 was only concerned with worker behavior on reconnects: it confirmed already paid invoices again.

This is about multiple push messages being sent per deposit. You can test in prod by depositing X sats. You should receive a deposit push notification that says you deposited 4*X sats.

Since only deposit push notifications seem to be broken, I think this is related to #726. But afaict, we correctly unsubscribe on errors1. And as mentioned, wasn't able to reproduce locally anymore.

Maybe we just need to restart the worker? When you deployed 70aa7dd, did you also restart the worker?

Or do you only restart it when there are changes to it? (I would guess the latter)

Footnotes

  1. and if reconnecting really does not work automatically, then this shouldn't even be possible

@huumn
Copy link
Member

huumn commented Jan 21, 2024

Everything rebuilds and restarts on every deployment.

@ekzyis
Copy link
Member Author

ekzyis commented Jan 21, 2024

Everything rebuilds and restarts on every deployment.

Mhh, I still get "4 sats deposited" when someone sends me 1 sat.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants