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

refactor(solana): remove dummy ws connection in SolanaClient #424

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

swimthor
Copy link
Contributor

@swimthor swimthor commented Oct 9, 2022

It passes the qa full test in https://exsphere.slack.com/archives/C02PVCS2PKQ/p1665410870060659?thread_ts=1665393074.911999&cid=C02PVCS2PKQ

To be honest I didn't see any actual code change in @solana/web3.js since #284 (comment). But I am align with Andrew this dummy ws is not needed because when we need the onSignature ws, it will create a new ws connection via https://github.com/solana-labs/solana-web3.js/blob/master/src/connection.ts#L5025-L5028

Notion ticket: https://www.notion.so/exsphere/Investigate-whether-we-still-need-a-dummy-websocket-connection-in-SolanaConnection-018975be2b1749a3b636644a07e2e5b9

Checklist

  • Github project and label have been assigned
  • Tests have been added or are unnecessary
  • Docs have been updated or are unnecessary
  • Preview deployment works (visit the Cloudflare preview URL)
  • Manual testing in #product-testing completed or unnecessary
  • New i18n strings do not contain any security vulnerabilities (e.g. should not allow translator to update email/url)
  • When fetching new translations from external sources, do a sanity check (e.g. get people who speak the language to check, shove all new translations into Google translate)

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Oct 9, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2820cd6
Status: ✅  Deploy successful!
Preview URL: https://05df999c.swim-ui.pages.dev
Branch Preview URL: https://test-if-need-dummy-connectio.swim-ui.pages.dev

View logs

@swimthor swimthor force-pushed the test-if-need-dummy-connection branch from 2bd05a3 to 96f6c0c Compare October 10, 2022 10:24
@swimthor swimthor changed the title [DO NOT MERGE] Test if need dummy connection refactor(solana): remove dummy ws connection in SolanaClient Oct 10, 2022
wormat
wormat previously approved these changes Oct 10, 2022
Copy link
Collaborator

@wormat wormat left a comment

Choose a reason for hiding this comment

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

OK let's give this another go but ask the testers to try again on swim.io once it's merged.

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.

2 participants