Skip to content

feat: full speck version implementation #1384

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

tabaktoni
Copy link
Member

@tabaktoni tabaktoni commented Apr 25, 2025

Motivation and Resolution

Expand subversion specification validation on provider instantiation with create().

  • SupportedRpcVersion is now the full spec version 0.8.1
  • [provider, channel].readSpecVersion() read recorded spec version from channel
  • [provider, channel].getSpecVersion()fetch from API full spec version
  • [provider, channel].setupSpecVersion() has functionality of old getSpecVersion(), return spec version if recorded else fetch, test for supported versions and store if fine, else throw error
  • RpcChannel.channelSpecVersion constant specifying for which specversion the channel is designed

Usage related changes

Moved version and resolution util methods from utils/provider to utils/resolve

Development related changes

Spec version is now in full form major.minor.patch (actual starknet spec is major.major.major)

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 marked this pull request as ready for review April 25, 2025 14:19
Copy link
Collaborator

@penovicp penovicp left a comment

Choose a reason for hiding this comment

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

A couple of rewording suggestions, otherwise lgtm

}

throw new LibraryError('Unable to detect specification version');
throw new LibraryError(
`Provided RPC node specification versions ${spec} is not compatible with the SDK. SDK supported RPC versions ${Object.keys(SupportedRpcVersion).toString()}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
`Provided RPC node specification versions ${spec} is not compatible with the SDK. SDK supported RPC versions ${Object.keys(SupportedRpcVersion).toString()}`
`Provided RPC node specification version ${spec} is not compatible with the SDK. SDK supported RPC versions: ${Object.keys(SupportedRpcVersion).toString()}`

readonly id = 'RPC071';

/**
* This Channel provide communication using spec version
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* This Channel provide communication using spec version
* RPC specification version this Channel class implements

This is a suggestion to sync the typedoc note with this.specVersion.

@@ -60,6 +66,9 @@ export class RpcChannel {

private chainId?: StarknetChainId;

/**
* Connected RPC node specification version
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Connected RPC node specification version
* RPC specification version of the connected node

This is a suggestion to sync the typedoc note with this. channelSpecVersion .

*/
public getSpecificationVersion() {
return this.fetchEndpoint('starknet_specVersion');
public async setupSpecVersion() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick, setup is usually used as a noun and set up as the corresponding verb form so the method would be setUpSpecVersion.

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