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

Test RPC default Providers Services #1061

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

Conversation

tabaktoni
Copy link
Collaborator

Motivation and Resolution

This issue #1058 motivated me to add a test for the default RPC version defined by lib and the default RPC version server by RPC.

This test does not ensure any security just print results.
Could we maybe add some CI warning or action for testing RPC versions?
We could set expectations in this two tests like

  • we expect 0.7 as the default version from RPC on base route
  • we expect 0.7 as the lib default version and all rpc should have 0.7 implemented

Some feedback @ivpavici @penovicp

RPC version (if applicable)

Ensure and Test RPC Providers

Checklist:

  • Performed a self-review of the code
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Linked the issues which this PR resolves
  • Documented the changes in code (API docs will be generated automatically)
  • Updated the tests
  • All tests are passing

@tabaktoni tabaktoni changed the title Test RPC Providers Test RPC default Providers Services Apr 3, 2024
@PhilippeR26
Copy link
Collaborator

There are 42 commits, with 36 files impacted!
Where is the code related to this verification of rpc spec?

@penovicp penovicp force-pushed the test/default-nodes-rpc-version branch from 6aa30bb to 18dcd3e Compare April 3, 2024 23:41
Object.keys(constants.RPC_NODES).map(async (network: any) => {
return Promise.all(
constants.RPC_NODES[network as keyof typeof constants.RPC_NODES].map(async (it: any) => {
const provider = new Provider({ nodeUrl: getBaseUrl(it) });
Copy link
Collaborator

Choose a reason for hiding this comment

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

except for getBaseUrl(it) the test here and above are the same, so we can extract it maybe and send as param!

@ivpavici
Copy link
Collaborator

ivpavici commented Apr 4, 2024

We could set expectations in this two tests like

we expect 0.7 as the default version from RPC on base route
we expect 0.7 as the lib default version and all rpc should have 0.7 implemented

I agree... also am curious for more thoughts!

@penovicp
Copy link
Collaborator

penovicp commented Apr 5, 2024

I think it's a good idea.

Might be worth considering doing this as a separate script since it would bypass the environment setup for the regular tests.

@PhilippeR26
Copy link
Collaborator

Just for information, Blast is responding to provider.getSpecVersion() with : 0.7.0-rc2

@PhilippeR26
Copy link
Collaborator

I understand that you want to check if all default networks (list hard coded in the project) are really using default_url_version.
All 4 url includes at the end the default_url_version :

SN_MAIN: [
`https://starknet-mainnet.public.blastapi.io/rpc/${RPC_DEFAULT_VERSION}`,
`https://free-rpc.nethermind.io/mainnet-juno/${RPC_DEFAULT_VERSION}`,
],
SN_SEPOLIA: [
`https://starknet-sepolia.public.blastapi.io/rpc/${RPC_DEFAULT_VERSION}`,
`https://free-rpc.nethermind.io/sepolia-juno/${RPC_DEFAULT_VERSION}`,
],

Logically, it should be OK by definition, but it's never bad to verify.
But I don't understand why you want to check the rpc versions of the base urls ... as we are not using them.

@tabaktoni tabaktoni added pending need update Type: feature New feature or request labels May 3, 2024
@ivpavici
Copy link
Collaborator

This will be probably revised and done through CI
cc @penovicp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending need update Type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants