Conversation
benjlevesque
left a comment
benjlevesque
left a comment
There was a problem hiding this comment.
Great work @alexandre-abrioux!!
packages/chain/src/chain-manager.ts
Outdated
| * Returns the list of supported chains | ||
| */ | ||
| get chains(): ChainTypes.IChain[] { | ||
| return Object.keys(this.ecosystems).reduce((chains, ecosystemName) => { |
There was a problem hiding this comment.
should this be stored in the instance?
| @@ -0,0 +1,12 @@ | |||
| import { EcosystemAbstract } from '../ecosystem-abstract'; | |||
| import { ChainTypes, RequestLogicTypes } from '@requestnetwork/types'; | |||
| import { chains } from './index'; | |||
There was a problem hiding this comment.
don't you have a circular dependency here? shouldn't the index for chains be in the chains/ folder?
| if (chains.length < 1) { | ||
| throw new Error( | ||
| `No chain found with "${propertyName}=${propertyValue}" for ecosystem(s) "${ecosystemsFilter}"`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
I have mixed feelings about throwing when a chain isn't found. It's convenient for types, but it can lead to unnecessary complexity if you want to check if a chain is supported.
For instance, we could imagine a dApp when we want to check in the ChainManager if the wallet's current chain is supported...
I think this is OK for now, we can review later if it proves problematic
| } | ||
| EvmChains.assertChainSupported(network); | ||
| return network; | ||
| return ChainManager.current().fromName(network, [ChainTypes.ECOSYSTEM.EVM]); |
There was a problem hiding this comment.
for this to work, it means setCurrent was called? (or the ChainManager will be empty) The request-node is an executable, not a library, so we can't set it before running the node
| }; | ||
|
|
||
| constructor(inputChains?: Record<ChainTypes.ECOSYSTEM, Record<string, ChainDefinition>>) { | ||
| if (!inputChains) return; |
There was a problem hiding this comment.
what's the purpose of this? in what case is it useful to have an emtpy ChainManager?
| this.legacyTokens = legacyTokens || CurrencyManager.getDefaultLegacyTokens(); | ||
| this.conversionPairs = conversionPairs || CurrencyManager.getDefaultConversionPairs(); | ||
| this.chainManager = chainManager || ChainManager.current(); | ||
| ChainManager.setCurrent(this.chainManager); |
There was a problem hiding this comment.
ha! this is the trick ;) So this is why it works in request-node.
There was a problem hiding this comment.
@alexandre-abrioux does that mean setCurrent should almost never be called directly?
As calling the constructor to CurrencyManager (or even getDefault would override it)
I feel this could lead to unexpected behaviours :thinking_face:
| ExtensionTypes.ID, | ||
| readonly ChainTypes.ECOSYSTEM[] | ||
| > = { | ||
| [ExtensionTypes.ID.CONTENT_DATA]: [ECOSYSTEM.EVM], |
There was a problem hiding this comment.
strange... CONTENT_DATA isn't a payment network...
| return extension; | ||
| } | ||
|
|
||
| public getExtensionsForChain( |
There was a problem hiding this comment.
any reason this was made public?
2 participants
Introducing a new package:
@requestnetwork/chain👋End Goal
"Dynamic chain injection"
Builders should be able to work with custom chains without modifying the Request Network library.
Constraints
1. Do not lose chain assertion
The different types of chains that we support, later referred to as "ecosystems", are currently strongly typed across our packages since (#1056) refactor: chain configurations. This was introduced to infer, by design, the compatibility of our codebase with specific chains.
The issue with the current solution is that our ecosystem types consist of a static list of chain names: see here. We cannot keep this design while allowing chain names to be dynamic.
I propose representing chains as objects and using different interfaces for chain ecosystems. This is a significant endeavor, as chains are currently described as strings across the codebase. However, we only need it in some places. Inputs and outputs (e.g. Request data) still contain strings and will not change. Only the internal logic should use objects, whenever possible, for strong typing of payment extensions and advanced logic.
The
@requestnetwork/smart-contractsand@requestnetwork/ethereum-storagepackages target only one ecosystem (EVMs). We don't need to represent chains as objects in these packages; type inference is unnecessary. The migration for this package was straightforward.2. Keep dependencies as small as possible
Chains are currently part of
@requestnetwork/currency, which contains multiple external dependencies. If we want to introduce logic for making chain description dynamic across the protocol, having a separate "light" package for handling chain configurations could be appropriate. To answer this, I've introduced@requestnetwork/chain, a package that should have no external dependency and can be safely included across our libraries.