WIP: Loss StarknetChainID #1223

BenFaruna · 2024-09-10T06:23:49Z

Motivation and Resolution

Users can set any custom L2/L3 chain ID
...

RPC version (if applicable)

...

Usage related changes

Change 1.
...

Development related changes

Change 1.
Changes in the chainId type from StarknetChainId to ChainId

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

BenFaruna · 2024-09-10T06:38:50Z

#1081
@ivpavici Changing the chain ID type from StarkentChainId has a ripple effect

About 15 files will need to be updated. Please let me know how you'd want me to move forward.

ivpavici · 2024-09-10T10:28:19Z

@BenFaruna yeah go for it!

BenFaruna · 2024-09-10T13:11:11Z

@ivpavici I have made the change to the codebase and substituted StarknetChainId for ChainId.

The image above shows the result after running the tests. It's the same result I've been getting before I started making any changes. Please review the change and let me know if I need to make any extra changes to the codebase.

BenFaruna · 2024-09-17T03:09:10Z

GM @ivpavici
Can I change this from a draft PR so it can be reviewed?

src/types/provider/configuration.ts

src/types/lib/index.ts

penovicp

Left some suggestions.

For future commits check the commit message guidelines.

penovicp · 2024-10-02T21:35:48Z

src/channel/rpc_0_6.ts

    return this.chainId;
  }

  public async getSpecVersion() {
-    this.specVersion ??= (await this.fetchEndpoint('starknet_specVersion')) as StarknetChainId;
+    this.specVersion ??= (await this.fetchEndpoint('starknet_specVersion')) as ChainId;


Suggested change

this.specVersion ??= (await this.fetchEndpoint('starknet_specVersion')) as ChainId;

this.specVersion ??= (await this.fetchEndpoint('starknet_specVersion'));

This looks like the cast can be removed. Also for rpc_0_7.

penovicp · 2024-10-02T21:37:10Z

src/types/lib/index.ts

@@ -313,4 +312,6 @@ export type ContractVersion = {
  compiler: CompilerVersion;
 };

+export type ChainId = '0x534e5f4d41494e' | '0x534e5f5345504f4c4941' | (`0x${string}` & {});


I would do this in a more verbose way:

Suggested change

export type ChainId = '0x534e5f4d41494e' | '0x534e5f5345504f4c4941' | (`0x${string}` & {});

export type StarknetChainId = ValuesType<{

SN_MAIN: '0x534e5f4d41494e'; // encodeShortString('SN_MAIN'),

SN_SEPOLIA: '0x534e5f5345504f4c4941'; // encodeShortString('SN_SEPOLIA')

}>;

export type ChainId = StarknetChainId | (`0x${string}` & {});

And also mark the existing StarknetChainId enum in src/constants.ts with /** @deprecated */.

Yea agree on extraction of values from type, but regarding deprecation, dev can still use enum to initialise known Starknet Chain Id's, so I would leave the enum (const) part of it.

penovicp · 2024-10-02T21:38:23Z

src/utils/starknetId.ts

@@ -132,7 +132,7 @@ export const StarknetIdContract = {
 /**
 * Returns the Starknet ID contract address based on the provided chain ID.
 *
- * @param {StarknetChainId} chainId The chain ID of the Starknet network.


I would use the StarknetChainId type from my src/types/lib/index.ts comment as the input parameter type for the methods in this file. Note that there would be a naming conflict with the existing enum so the enum would need to be aliased.

Using the StarknetChainId in your previous comment as the input parameter type restricts the input value for functions that depend on it. This becomes an issue in src/provider/extensions/starknetId.ts because the getChainId function returns ChainId and not StarknetChainId.
Should I still go ahead with this change? @penovicp

Yes. Cast the getChainId calls as StarknetChainId in that file.

Changing the input type from ChainId to StarknetChainId for the function in src/utils/starknetId.ts will result in changing some of the interfaces. After that, the code returns to its original state, allowing only the chain ID of mainnet and sepolia testnet. That defeats the purpose of this change
What do you think?

I'm not sure what you mean.

The code for these starknetId utilities is hardcoded to only produce valid results for the mainnet and sepolia chain ids and it throws an error otherwise, this is why I said to use the more restricted type for the input. This PR should allow for a looser chain id to be used within the core of starknet.js such as the Account, Contract, Provider, etc. classes and keep it restricted for the starknetId code. From what I can see there are no starknetId specific interfaces so none should be affected.

I understand better now. Thank you @penovicp

tabaktoni

waiting for resolution

tabaktoni · 2024-10-31T11:00:36Z

The initial idea was to open for L3 chains, but once this request is closed, we can close this action.
For L3 chains We could create separate rpc channels.
The second resolution was from get-starknet but in the domain of starknet only defined chains are allowed.

ivpavici · 2024-10-31T11:41:43Z

@BenFaruna thanks for your effort! sent you a small reward for participation!

BenFaruna · 2024-10-31T14:51:58Z

@BenFaruna thanks for your effort! sent you a small reward for participation!

Sorry I couldn't complete, I got swarmed after the initial commits 😓
Thank you for the consideration, I do appreciate it

BenFaruna changed the title ~~WIP: Loss StarknetChainID~~ WIP: Loss StarknetChainID #1081 Sep 10, 2024

BenFaruna changed the title ~~WIP: Loss StarknetChainID #1081~~ WIP: Loss StarknetChainID Sep 10, 2024

BenFaruna force-pushed the feat/loss-chainid branch from 2fce1c8 to 72cd2d2 Compare September 12, 2024 12:27

ivpavici reviewed Oct 1, 2024

View reviewed changes

src/types/provider/configuration.ts Outdated Show resolved Hide resolved

tabaktoni reviewed Oct 1, 2024

View reviewed changes

src/types/lib/index.ts Outdated Show resolved Hide resolved

BenFaruna added 3 commits October 2, 2024 03:08

feat: create the ChainId type

8d7b52f

feat: substitute the use of StarknetChainId for ChainId

cfa3a7c

feat: chain id support existing starknet chainId and custom ones

6afdcce

BenFaruna force-pushed the feat/loss-chainid branch from a5012d6 to 6afdcce Compare October 2, 2024 02:22

BenFaruna marked this pull request as ready for review October 2, 2024 02:22

BenFaruna requested review from tabaktoni and ivpavici October 2, 2024 02:22

ivpavici requested a review from penovicp October 2, 2024 11:58

penovicp reviewed Oct 2, 2024

View reviewed changes

tabaktoni requested changes Oct 8, 2024

View reviewed changes

tabaktoni closed this Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Loss StarknetChainID #1223

WIP: Loss StarknetChainID #1223

BenFaruna commented Sep 10, 2024 •

edited

Loading

BenFaruna commented Sep 10, 2024

ivpavici commented Sep 10, 2024

BenFaruna commented Sep 10, 2024

BenFaruna commented Sep 17, 2024

penovicp left a comment

penovicp Oct 2, 2024

penovicp Oct 2, 2024

tabaktoni Oct 3, 2024

penovicp Oct 2, 2024

BenFaruna Oct 3, 2024

penovicp Oct 3, 2024

BenFaruna Oct 4, 2024

penovicp Oct 7, 2024

BenFaruna Oct 8, 2024

tabaktoni left a comment

tabaktoni commented Oct 31, 2024

ivpavici commented Oct 31, 2024

BenFaruna commented Oct 31, 2024

	this.specVersion ??= (await this.fetchEndpoint('starknet_specVersion')) as ChainId;
	this.specVersion ??= (await this.fetchEndpoint('starknet_specVersion'));

-export type ChainId = '0x534e5f4d41494e' | '0x534e5f5345504f4c4941' | (`0x${string}` & {});
+export type StarknetChainId = ValuesType<{
+  SN_MAIN: '0x534e5f4d41494e'; // encodeShortString('SN_MAIN'),
+  SN_SEPOLIA: '0x534e5f5345504f4c4941'; // encodeShortString('SN_SEPOLIA')
+}>;
+export type ChainId = StarknetChainId | (`0x${string}` & {});

WIP: Loss StarknetChainID #1223

WIP: Loss StarknetChainID #1223

Conversation

BenFaruna commented Sep 10, 2024 • edited Loading

Motivation and Resolution

RPC version (if applicable)

Usage related changes

Development related changes

Checklist:

BenFaruna commented Sep 10, 2024

ivpavici commented Sep 10, 2024

BenFaruna commented Sep 10, 2024

BenFaruna commented Sep 17, 2024

penovicp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tabaktoni left a comment

Choose a reason for hiding this comment

tabaktoni commented Oct 31, 2024

ivpavici commented Oct 31, 2024

BenFaruna commented Oct 31, 2024

BenFaruna commented Sep 10, 2024 •

edited

Loading