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 websocket confirmation options handling #4855

Conversation

gr0vity-dev
Copy link
Contributor

This PR fixes an issue with the websocket confirmation message handling. Previously, the system would cache messages based solely on the include_block option, causing other subscriber options (like include_linked_account and include_sideband_info) to be ignored if another subscriber had already created a cached message.

Changes made:

  • Removed the message caching mechanism that was causing options to be ignored
  • Each subscriber now receives a message built specifically with their configuration options
  • Simplified parameter naming for better readability
  • Made options handling more consistent across the codebase
  • Added a failing testcase that is passing by the removal of the cache

These changes ensure that all subscribers receive confirmation messages that correctly respect their individual option settings.

@clemahieu clemahieu added this to the V28 milestone Mar 4, 2025
@gr0vity-dev gr0vity-dev force-pushed the prs/remove_websocket_confirmation_topic_cache branch from db8fbcb to 0c6645a Compare March 4, 2025 19:23
@gr0vity-dev-bot
Copy link

gr0vity-dev-bot commented Mar 4, 2025

Test Results for Commit 0c6645a

Pull Request 4855: Results
Overall Status:

Test Case Results

  • 5n4pr_conf_10k_bintree: PASS (Duration: 109s)
  • 5n4pr_conf_10k_change: PASS (Duration: 136s)
  • 5n4pr_conf_change_dependant: FAIL (Duration: 576s)
  • Log
  • 5n4pr_conf_change_independant: PASS (Duration: 139s)
  • 5n4pr_conf_send_dependant: PASS (Duration: 126s)
  • 5n4pr_conf_send_independant: PASS (Duration: 118s)
  • 5n4pr_rocks_10k_bintree: PASS (Duration: 107s)
  • 5n4pr_rocks_10k_change: PASS (Duration: 174s)

Last updated: 2025-03-04 20:33:58 UTC

@gr0vity-dev
Copy link
Contributor Author

This was previous code + testcase only:

Screenshot 2025-03-04 at 20 33 25

Here is the run

@pwojcikdev pwojcikdev merged commit 2c7c736 into nanocurrency:develop Mar 6, 2025
24 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged / V28.0
Development

Successfully merging this pull request may close these issues.

4 participants