feat: agents.fun in celo (UI configs) #647
Conversation
…ype safety and clarity
mohandast52
changed the title
| @@ -15,11 +15,12 @@ const { Text } = Typography; | |||
| * update as needed; check https://app.safe.global/new-safe/create for prefixes | |||
| */ | |||
| const safeChainPrefix = { | |||
| [EvmChainId.Ethereum]: 'eth', | |||
| [AllEvmChainId.Ethereum]: 'eth', | |||
There was a problem hiding this comment.
why do you think we need two separate sets of evmChainIds? wouldn't that be confusing?
There was a problem hiding this comment.
This is due to the “EvmChainId” type. In several places, we were using EvmChainId | number, which isn’t ideal as it allows any valid number. To make it stricter, we need to limit it to the specific chain IDs we support. AllEvmChainId (possibly a better name could be chosen) could simply act as an enum holding these chain IDs. Personally, I’d prefer to stick to EvmChainId, but for safeChainPrefix, we might require a more generic type. Let me know your thoughts.
There was a problem hiding this comment.
I honestly think we can use the same service class for both, we pass chainId anyway to each function. wdyt?
There was a problem hiding this comment.
Yeah, I think so too. That’s why I added a TODO to address it later, but I’ll see if I can do something about it now and create a common class.
There was a problem hiding this comment.
couldn't we now just simply use frontend/service/agents/base-services/AgentsFun.ts instead of both frontend/service/agents/AgentsFunCelo.ts and frontend/service/agents/AgentsFunBase.ts?
There was a problem hiding this comment.
We could use the same class, but it’s better to have a separate class for each service because it is be easy to understand, keeping it modular and debug going forth (Modularity and Maintainability). Also, this way, we can handle any slight variations in logic without affecting others (Scalability). Hence, I’d prefer keeping two different but similar classes.
| import { formatEther } from 'ethers/lib/utils'; | ||
|
|
||
| import { STAKING_PROGRAMS } from '@/config/stakingPrograms'; | ||
| import { CELO_STAKING_PROGRAMS } from '@/config/stakingPrograms/celo'; |
There was a problem hiding this comment.
re above, just maybe need to make this abstract and make chainId param required everything instead of falling back to default
| description: 'Memeooorr @twitter_handle', // should be overwritten with twitter username | ||
| service_version: 'v0.2.0-alpha16', |
There was a problem hiding this comment.
still think these two could be in AGENTS_FUN_COMMON_TEMPLATE
| }): Promise<StakingRewardsInfo | undefined> => { | ||
| if (!chainId) throw new Error('ChainId is required'); |
There was a problem hiding this comment.
wouldn't ts prevent us from passing not a EvmChainId value? so it can't be null/0?
applicable to all similar places below
There was a problem hiding this comment.
TypeScript doesn’t perform runtime checks, so it’s better to include this to ensure reliability. If the line is executing within "chain-specific" classes, I would want the chain ID to always be present.
There was a problem hiding this comment.
can we rename the base-services to common? or shared? to me it sounds like related to base chain
There was a problem hiding this comment.
couldn't we now just simply use frontend/service/agents/base-services/AgentsFun.ts instead of both frontend/service/agents/AgentsFunCelo.ts and frontend/service/agents/AgentsFunBase.ts?
Proposed changes
Types of changes
What types of changes does your code introduce?
Put an
xin the boxes that apply