-
Notifications
You must be signed in to change notification settings - Fork 91
refactor: chain configurations #1056
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
Conversation
| * @param networkId Id of the network | ||
| * @return name of the network | ||
| */ | ||
| export default { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the default export here (cc @MantisClone) and exported these utilities in the index file of the package. It allows other packages to filter storage chains amongst all supported EVM chains.
| const network = getEthereumStorageNetworkNameFromId(config.getStorageNetworkId()); | ||
| if (!network) { | ||
| throw new Error(`Storage network not supported: ${config.getStorageNetworkId()}`); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how the newly exported ethereum-storage utility becomes convenient.
|
|
||
| /** Parameters for the creation action */ | ||
| export interface ICreationParameters extends PnAnyToAnyConversion.ICreationParameters { | ||
| network?: EvmChainName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
precise network type for pn-any-to-erc20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to know if @leoslr thinks this is compatible with the approach of calling all fungible tokens ERC20, even on non-EVMs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't be compatible no.
Currently the tests are pasing as aurora and aurora-testnet are declared as EvmChainName.
I think we should keep the ChainName from the underlying interface. Calling all fungible token ERC20 is a langage abuse, but in the context of creating requests and detecting the payment, the logic remains the same.
benjlevesque
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great job on the chain configuration 👏
I'm not totally convinced by the EvmChainName though... Imo, we could consider chains as a purely configurable: in the future, chain configuration could even include smart contract addresses, and could be injected to the client. That would make typing the chain name irrelevant, and even wrong.
I think you cover the problem very well with the assertChainSupported util, and it's probably enough.
|
(saw on a call with @benjlevesque) Use case 1
This use case will be voided once we implement a proper way of dynamically configuring chains. Like @benjlevesque said:
At this point, we should implement all safeguards at run time (when the library is first loaded, for instance), which would replace those compile-time safeguards. Use case 2
This is also a nice to have, but having a union type is not future-proof (it's not compatible with a dynamic configuration). What we could do is implement proper classes to represent chains in our codebase, instead of representing them as strings. That way:
The feature-shielding could be preserved. The assertion of compatibility would be made when first creating the chain class, something like: class EvmChain implements {
contructor (public name) {
// here we would check that the chain is supported from the dynamic chain configuration
assertChainSupported(name);
}
}ConclusionOverall, those types were hard to add but will be very easy to replace by Please let me know if you have other concerns or suggestions! MiscA suggestion was to use the newly added Unfortunately, this is not easily doable: chain configurations are declared in |
yomarion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(reviewed advanced-logic, currency, types, toolbox and smart-contracts for now)
packages/currency/src/native.ts
Outdated
| import { NamedNativeCurrency } from './types'; | ||
|
|
||
| export const nativeCurrencies: Record<NativeCurrencyType, (NativeCurrency & { name: string })[]> = { | ||
| type NativeEthCurrency = NamedNativeCurrency & { network: CurrencyTypes.EvmChainName }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1/ Generic question: why having a different type per blockchain type?
2/
Watch out:
{
symbol: 'NEAR',
decimals: 24,
name: 'Near',
network: 'aurora',
},
Was defined within RequestLogicTypes.CURRENCY.ETH, as it seemed complex to define one Currency type per chain. These new types can enforce the confusion, maybe it's easier to make the difference at this stage with NativeNearCurrency (but cf. 1/).
Option 1/n: NativeAMCurrency, AM standing for AccountModel, opposed to UTXO (https://www.horizen.io/blockchain-academy/technology/expert/utxo-vs-account-model/#:~:text=The%20UTXO%20model%20is%20a,constructions%2C%20as%20well%20a%20sharding.)
Option 2/n: NativeVMCurrency: Common type for VM chains native tokens
...
3/ Consider usign Evm instead of Eth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it in d6ae451 with the following bit of code:
type NativeEthCurrency = NamedNativeCurrency & {
network: CurrencyTypes.EvmChainName | CurrencyTypes.NearChainName;
};why having a different type per blockchain type
The goal was to have a different type per "currency type".
IMO, if we can specify the type, let's do it; it brings clarity to the model. The change already revealed small flaws in the current design (which are OK flaws):
- flaw 1: because chain names are not unique across different "chain types", a collision is possible for "currency types" that span across multiple "chain types", like
RequestLogicTypes.CURRENCY.ETHthat spans across EVM-type and NEAR-type chains - flaw 2: the "currency type"
RequestLogicTypes.CURRENCY.ETH's name is a bit confusing
I think that's great: it made it more obvious. I didn't want to change any logic in this PR, so I haven't refactored anything related to currencies. Let's keep it in mind tho! I didn't know about "UTXO vs. Account Model", thanks!
Description of the changes
This PR adds chain configuration files. Those configs allow us to easily convert Request Network's internal chain names (
mainnet,aurora, etc...) to standard chain IDs.This PR also strengthens the types of chain names across our packages. Chain names used to be simple strings. They are now strong types, and it is easier to know which part of the protocol is compatible with EVM chains, and, which part is compatible with a broader definition of chains.
How to review
I'd suggest checking first those new files before anything else:
packages/types/src/currency-types.tspackages/currency/src/chains/evm/index.tspackages/currency/src/chains/btc/index.tspackages/currency/src/chains/index.tsThe rest is mostly changes of types.
Possible Improvements (next steps)
We could strongly type storage chains = have a different type for storage chains instead of using
CurrencyType.EvmChainName.