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

Improved constants #28

Merged
merged 14 commits into from
Jun 21, 2024
Merged

Improved constants #28

merged 14 commits into from
Jun 21, 2024

Conversation

Keyrxng
Copy link
Member

@Keyrxng Keyrxng commented Jun 15, 2024

Resolves #27

couple of changes made

  • In order to improve our exported types I've moved to dynamically generating types and constants

  • I've also written the extraRpcs from /lib/chainlist into an object, this allows us to work with the list directly without esbuild complaining, likely removes the need for node-fetch too (will check)

  • Because the extraRpcs list is being written to a local object we no longer need to inject them at build

  • Improved RPCHandler and config with updated types

  • added tests covering the two helper functions and the Records

  • added a bunch of explorers and native tokens (18 decimals is the standard, it's EVM breaking otherwise, I have not verified each)

  • Programmatically collecting explorers, native crypto info etc from the same source that chainList uses. This approach also ensures that we can provide RPC endpoints for testnets because the extraRPCs object that came from /lib/chainlist originally had only mainnets.

@Keyrxng
Copy link
Member Author

Keyrxng commented Jun 15, 2024

26k additions lmao, keyrxng strikes again

ofc it's because of the types and consts being generated.


Originally I went out and manually collected explorers but in order to support them at a type level I'd need to include them in the base ChainId and ChainName types. Doing that would give the false impression that the RPC handler could support those chains and return a provider when it would only support returning an explorer from the const.

So the best solution was to use the same data source as chainlist but realised they exposed a handy function for generating chain data which removed more than a handful of chains/chainIds that were causing problems at the type level.

Previously I was allowing for loose auto-complete but the expectation now is that there isn't a chain we can't support so long as it has RPCs.

@Keyrxng Keyrxng requested a review from 0x4007 June 15, 2024 21:30
@Keyrxng Keyrxng requested a review from molecula451 June 16, 2024 00:14
@0x4007
Copy link
Member

0x4007 commented Jun 16, 2024

Resolves #27

couple of changes made

  • In order to improve our exported types I've moved to dynamically generating types and constants

  • I've also written the extraRpcs from /lib/chainlist into an object, this allows us to work with the list directly without esbuild complaining, likely removes the need for node-fetch too (will check)

  • Because the extraRpcs list is being written to a local object we no longer need to inject them at build

What was the problem with injecting at build? Seems that the codebase was significantly smaller and more maintainable before. You did this to remove the one tsc ignore?

  • Improved RPCHandler and config with updated types

  • added tests covering the two helper functions and the Records

  • added a bunch of explorers and native tokens (18 decimals is the standard, it's EVM breaking otherwise, I have not verified each)

I don't understand this but we shouldn't assume the decimals. Besides USDC/T use six and are very popular tokens to work with.

  • Programmatically collecting explorers, native crypto info etc from the same source that chainList uses. This approach also ensures that we can provide RPC endpoints for testnets because the extraRPCs object that came from /lib/chainlist originally had only mainnets.

@Keyrxng
Copy link
Member Author

Keyrxng commented Jun 16, 2024

What was the problem with injecting at build? Seems that the codebase was significantly smaller and more maintainable before. You did this to remove the one tsc ignore?

When trying to work with the export from chainlist esbuild would throw because it would need node specific modules that would need injected. So it was either bundle them through esbuild and only access them via a declared const, inject the required modules it complained about or remove the node dependency all together. Because this is expected to run in any env, I chose removing it all together.

But beyond that, I was not able to coerce the JSON values into literal string types see this issue. The writing of the contents to our TS file let's us create far better type exports.

It also dramatically increases the number of chains that are supported by this package because it also includes testnets

The generated types and consts make up 25,951 additions, the additional code written is only ~320 odd.

I don't understand this but we shouldn't assume the decimals. Besides USDC/T use six and are very popular tokens to work with.

A native token such as ETH should always be 18 decimals which is what that exports contains. Because ERC20 rewards tokens can be any arb value their decimal handling should be done on the fly and considering everything is moving towards full ERC20 configurability, does it make sense to hardcode tokens across various chains or even export something like that from this pkg?

While it's possible for a chain's native crypto to be a different decimal it is not EVM standard

@0x4007
Copy link
Member

0x4007 commented Jun 17, 2024

Okay your rationale makes sense. Just as long as we support the chains with permit2 regarding the 18 decimals it's fine. I'm assuming you're referring only to the native token of the networks.

build/types.ts Outdated
}

// Clear the file
await writeFile("types/dynamic.ts", "/* eslint-disable sonarjs/no-duplicate-string */\n\n");
Copy link
Member

Choose a reason for hiding this comment

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

Instead of ignoring the duplicate string rule, why don't you assign this to a variable and reuse?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not a feasible approach because the file is 29k in length. The duplicates are things like long and short form crypto names, chain names etc.

P.S a cryptocurrency technically speaking is the native crypto (gas token) of that network and all other "cryptos" are tokens on the network or at least that distinction used to be valid.

Copy link
Member

@0x4007 0x4007 Jun 18, 2024

Choose a reason for hiding this comment

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

I'm referring to the file name.

Colloquially the "coin" is the gas token. Everything else refers to erc20 compliant tokens. Cryptocurrency, colloquially refers to both "coins" and "tokens"

Although I haven't heard anybody make a point about distinction since 2018.

Copy link
Member Author

@Keyrxng Keyrxng Jun 18, 2024

Choose a reason for hiding this comment

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

Ah I misunderstood, ignoring the rule was for contents of the file but the name would have thrown eslint as well.

I know they are all used interchangeably now, it can be easy to misconstrue sometimes

build/types.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/services/rpc-service.ts Outdated Show resolved Hide resolved
tests/mocks/rpc-handler.ts Show resolved Hide resolved
tests/script-test.ts Outdated Show resolved Hide resolved
types/rpc-handler.ts Show resolved Hide resolved
build/dynamic-types.ts Outdated Show resolved Hide resolved
build/esbuild-build.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@Keyrxng Keyrxng requested a review from 0x4007 June 20, 2024 14:01
return this._networkId;
}

public getNetworkName(): string {
public getNetworkName() {
Copy link
Member

@molecula451 molecula451 Jun 20, 2024

Choose a reason for hiding this comment

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

why remove the return type ?

Copy link
Member Author

Choose a reason for hiding this comment

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

because the class values are typed there is no need to explicitly annotate a return type here as nothing is mutated in the function. Removal of string specifically is because it now uses the NetworkName union type.

@0x4007 0x4007 merged commit 30022f4 into ubiquity:development Jun 21, 2024
3 checks passed
@ubiquibot ubiquibot bot mentioned this pull request Jun 21, 2024
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.

Improved constants
3 participants