feat: baseTokens & chainIds #43
Conversation
ZKS-151 Get chainId's
In order to get chain ids we should add ZK sync era StateTransitionManager: 0xc2eE6b6af7d616f6e27ce7F4A451Aedc2b0F5f5C Use this method to get all chainIds from StateTransitionManager: https://github.com/matter-labs/era-contracts/blob/8a70bbbc48125f5bde6189b4e3c6a3ee79631678/l1-contracts/contracts/state-transition/StateTransitionManager.sol#L109 Steps:
ZKS-152 Get baseToken
we need to get `baseToken` given a chain Id. use |
| @@ -716,6 +731,86 @@ describe("L1MetricsService", () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe("getChainIds", () => { | |||
| it("returns chainIds", async () => { | |||
| true && true; | |||
There was a problem hiding this comment.
Is this some kind of weird hack or is it something that was left by accident? 🔎
| }), | ||
| allowFailure: false, | ||
| }); | ||
| this.chainIds = chainIds.flat(); |
There was a problem hiding this comment.
Dropping this comment just in case, from my ignorance on how/when is this service going to be instantiated.
If this is going to be used while answering requests and the chainIds are going to change at some point, let's be sure that this service is instantiated once per request or something like that, to avoid having a deprecated value of chainIds as there's no mechanism to renew them. This issue would arise, for example, if this service is instantiated only when the NestJS app starts and is reused with all the requests.
There was a problem hiding this comment.
i think that chainIds won't change frequently enough (for now) but it's a good point. maybe when implementing Cache we could update this implementation
There was a problem hiding this comment.
what we are gona do is to implement an Inmemory cache during milestone Enhancements, almost every method of the L1MetricsService will have a caching strategy. In particular for getChainIds what we plan is to have a ttl of x minutes, that will ensure that the values will be updated and not get stuck.
|
|
||
| expect(result).toEqual(mockedMulticallReturnValue); | ||
| }); | ||
| it("returns baseTokens", async () => { |
There was a problem hiding this comment.
| it("returns baseTokens", async () => { | |
| it("returns empty array if chainIds is empty", async () => { |
| it("returns empty array if chainIds are empty", async () => { | ||
| const mockedChainIds: bigint[] = []; | ||
| l1MetricsService = await mockMetricsModule( | ||
| mockedBridgeHubAddress, | ||
| mockedSharedBridgeAddress, | ||
| mockedSTMAddresses, | ||
| ); | ||
| l1MetricsService["chainIds"] = mockedChainIds; | ||
|
|
||
| const result = await l1MetricsService.getChainIds(); | ||
|
|
||
| expect(result).toEqual(mockedChainIds); | ||
| }); |
There was a problem hiding this comment.
I feel like you wanted to test this scenario in getBaseTokens but ended up doing it in the getChainIds. This case might be already covered by the returns chainIds previously setted up on the returns chainIds previously setted up
There was a problem hiding this comment.
it was on purpose hehehehe, but yea,returns chainIds previously setted up should cover the case, just wanted to test this edge case in particular.
| }), | ||
| allowFailure: false, | ||
| }); | ||
| this.chainIds = chainIds.flat(); |
There was a problem hiding this comment.
i think that chainIds won't change frequently enough (for now) but it's a good point. maybe when implementing Cache we could update this implementation
| * Get the base token for each chain | ||
| * @returns A map of chainId to base token address | ||
| */ | ||
| async getBaseTokens(chainIds: ChainId[]): Promise<Address[]> { |
There was a problem hiding this comment.
should we return a Token from the whitelisted from Coingecko instead of plain Address?
ie. search in the static token list the one that matches the contractAddress (or Ethereum if adresss(1)) so metadata is returned to frontend too (with image url and all that stuff)
There was a problem hiding this comment.
niceeee, lets do it
| it("returns known tokens", async () => { | ||
| const mockedChainIds = [1n, 2n]; | ||
| const knownTokenAddress1 = Object.keys(erc20Tokens)[0]; | ||
| const knownTokenAddress2 = Object.keys(erc20Tokens)[0]; |
There was a problem hiding this comment.
Is the duplicated usage of the [0] element intended?
| @@ -0,0 +1,4 @@ | |||
| import { Token, TokenType } from "@zkchainhub/shared/types"; | |||
|
|
|||
| export const isNativeToken = (token: Token<TokenType>): token is Token<"native"> => | |||
| decimals: 18, | ||
| name: "unknown", | ||
| type: "erc20", | ||
| symbol: "unknown", |
There was a problem hiding this comment.
we can add a task to enhancements to fetch decimals, name and symbol from the contract now that i think of it but for now it's good since we know that baseTokens are permisioned and practically it'll be Ether most of the times
🤖 Linear
Closes ZKS-151 ZKS-152
Description
getBaseTokensgetChainIdsbridgehubAddress&sharedBridgeAddressfromshared/constants.tsbridgehubAddress,sharedBridgeAddress&stateTransitionManagerAddressas parameters forL1MetricsService