-
Notifications
You must be signed in to change notification settings - Fork 300
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
blockSubscribe RPC Method #416
Conversation
Hi, thanks for opening a PR! Would you mind fixing the lint issue and also writing a test? You can run solana-py/tests/integration/test_websockets.py Lines 283 to 292 in 53adfce
|
Hi, thanks for checking. I just added a simple test for slot and used |
Test not passing maybe because not all RPCs support this method:
|
Hi @ruiqic. You can add the flag here: solana-py/tests/docker-compose.yml Line 11 in 53adfce
|
Also, would you mind reverting all the formatting changes not related to this PR? |
src/solana/rpc/websocket_api.py
Outdated
""" | ||
req_id = self.increment_counter_and_get_id() | ||
commitment_to_use = None if commitment is None else _COMMITMENT_TO_SOLDERS[commitment] | ||
encoding_to_use = None if encoding is None else _ACCOUNT_ENCODING_TO_SOLDERS[encoding] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we need to use _TX_ENCODING_TO_SOLDERS
instead:
encoding_to_use = None if encoding is None else _ACCOUNT_ENCODING_TO_SOLDERS[encoding] | |
encoding_to_use = None if encoding is None else _TX_ENCODING_TO_SOLDERS[encoding] |
solana-py/src/solana/rpc/core.py
Lines 93 to 99 in 53adfce
_TX_ENCODING_TO_SOLDERS = { | |
"binary": UiTransactionEncoding.Binary, | |
"base58": UiTransactionEncoding.Base58, | |
"base64": UiTransactionEncoding.Base64, | |
"json": UiTransactionEncoding.Json, | |
"jsonParsed": UiTransactionEncoding.JsonParsed, | |
} |
Hi, thanks for checking the code. How can the coverage test be fixed? |
I'll fix that separately. @ruiqic, would you mind giving me push access to your forked repo? I just want to push some clean up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
Thanks. I already had |
No description provided.