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

use base64+zstd for account data encoding by default, add ping support and make recv cancellation safe #18

Merged
merged 5 commits into from Mar 26, 2022

Conversation

dovahcrow
Copy link
Contributor

@dovahcrow dovahcrow commented Feb 16, 2022

Sorry I squeezed the fixes for #17 #19 and a base64+zstd fix into the same PR.

The first commit in the PR fixes the following problem.
The jsonParsed encoding won't work if the account data can be parsed by the RPC node. For example, the spl-token account will be actually parsed! The notification from WS becomes

{
    "jsonrpc": "2.0",
    "method": "accountNotification",
    "params": {
        "result": {
            "context": {
                "slot": 114915064
            },
            "value": {
                "lamports": 2039280,
                "data": {
                    "program": "spl-token",
                    "parsed": {
                        "info": {
                            "isNative": false,
                            "mint": "USDhTjkUXFfigLELiFpbBnpLmEm4aXHvdY2kDSadJDH",
                            "owner": "8dyR4YwFHzh2Pxmgs9TPst5Mu4kkES5tu8w6SuhdEyUG",
                            "state": "initialized",
                            "tokenAmount": {
                                "amount": "5771086231612606",
                                "decimals": 9,
                                "uiAmount": 5771086.231612606,
                                "uiAmountString": "5771086.231612606"
                            }
                        },
                        "type": "account"
                    },
                    "space": 165
                },
                "owner": "TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA",
                "executable": false,
                "rentEpoch": 266
            }
        },
        "subscription": 10447
    }
}

and cannot be deserialized into the AccountRepresentation struct.

Choosing base64+zstd because it is compressed and should save network bandwidth theoretically (which is the bottleneck).

The rest two commits fixes #17 and #19.

@dovahcrow dovahcrow changed the title use base64+zstd for account data encoding by default use base64+zstd for account data encoding by default, add ping support and make recv cancellation safe Feb 16, 2022
@y2kappa
Copy link
Contributor

y2kappa commented Feb 17, 2022

@boymaas @karim-agha

@y2kappa
Copy link
Contributor

y2kappa commented Feb 17, 2022

@dovahcrow let us know when it’s ready

@dovahcrow
Copy link
Contributor Author

@dovahcrow let us know when it’s ready

It's ready.

@y2kappa y2kappa requested review from boymaas, karim-agha and y2kappa and removed request for karim-agha March 7, 2022 11:01
Copy link
Contributor

@karim-agha karim-agha left a comment

Choose a reason for hiding this comment

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

Not a very thorough review on my part, but lgtm on first sight. Maybe it's good to have one more pair of eyes looking at it.

@y2kappa
Copy link
Contributor

y2kappa commented Mar 17, 2022

@boymaas @oeble ping

@boymaas
Copy link
Contributor

boymaas commented Mar 17, 2022

Looks good, a keep alive and base64+zstd. Approved from my part

Copy link
Contributor

@y2kappa y2kappa left a comment

Choose a reason for hiding this comment

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

Cargo.toml Outdated
license = "Apache-2.0"
name = "solana-shadow"
repository = "https://github.com/hubble-markets/solana-shadow"
version = "0.2.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

please bump the version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@y2kappa y2kappa merged commit 80a3252 into hubbleprotocol:master Mar 26, 2022
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.

Consider do json rpc ping instead of reconnect every now and then?
5 participants