-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add support to multiple networks #785
base: main
Are you sure you want to change the base?
Conversation
Hey, appreciate the comment, @Shadowfiend! This implementation requires a few new environment variables to be added to the .env file for the app to run with the default configuration. Additionally, there are some variables that need to be removed, as shown in the following links: https://github.com/threshold-network/token-dashboard/pull/785/files#diff-9bd30daf6ec3456f77aa9820a3b9d6f37df613de9123d6ed3085a32318b03f7dL89-R99 Basically, the environment variables that have to be added are:
|
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.
Oi, lost track of this fellow. Can you send me the relevant secrets via Keybase? I'm at https://keybase.io/shadowfiend, you can xref my GitHub proof there.
|
||
const MAIN_ALCHEMY_URL = "g.alchemy.com/v2/" | ||
|
||
export const getRpcUrl = (chainId?: number | string) => { |
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 don't love that we've baked Alchemy in so deep, fwiw (though not a blocker).
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.
The connection to the RPC URL has become dynamic with the inclusion of support for connecting to different networks. Therefore, the implemented solution, as can be seen below, is that the RPC url changes dynamically according to the connected network, following the service pattern, for example, if the connected network is Arbitrum on Sepolia, the RPC url would be 'https://arbitrum-sepolia.g.alchemy.com/v2/SERVICE-API'. Other services, as Infura, might offer various RPC urls on demand based on the connected networks, but it's expected that the pattern would change.
We can expand the possibilities of different services provided, but some sort of check should be done in this regard. If you think this should be changed, do you have any suggestions for an approach? For example, we could include an environment variable that determines the type of service—e.g., RPC_SERVICE_NAME: 'Alchemy', 'Infura', etc.—and perform validation through an enum providing different URL patterns. Or, alternatively, perhaps a more advanced validation could be done against the API pattern being passed as a parameter in the RPC API in the env file?
token-dashboard/src/networks/utils/getRpcUrl.ts
Lines 8 to 17 in 63705c7
export const getRpcUrl = (chainId?: number | string) => { | |
const alchemyApi = getEnvVariable(EnvVariable.ALCHEMY_API) | |
const defaultChainId = SupportedChainIds.Ethereum | |
const chainIdNum = Number(chainId) || defaultChainId | |
const alchemyConfig = networksAlchemyConfig[chainIdNum] | |
return alchemyConfig?.name | |
? `https://${alchemyConfig.name}-${alchemyConfig.type}.${MAIN_ALCHEMY_URL}${alchemyApi}` | |
: `http://localhost:8545` | |
} |
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 also agree that I don't love how deeply entrenched Alchemy is in the code eg. alchemyName
in the network definition. Down the road if we decide to switch away from Alchemy, it would be tricky to replace with some other RPC service like Infura or dRPC.
I'm not familiar with the code, so just speculating, but perhaps you could have a generic API_KEY
and then an additional API_SERVICE_NAME
variable and a mapping of rpc service name -> rpc config, and the configs have properties/methods to provide/adjust the required names/values/templated strings accordingly. Adding a supported service would involve adding a config entry to the mapping...?
That being said, that can probably be addressed/investigated in a separate PR.
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 tend to agree that this can be better structured based on multiple possible services, so I opened the following issue to track this #790, and I'm going to address this in a separate PR.
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.
Nice job. Here are my comments thus far - I'm about halfway through the changes 😅.
getDurationByNumberOfConfirmations(confirmations) | ||
// Round up the minutes to the nearest half-hour | ||
const hours = (Math.round(durationInMinutes / 30) * 30) / 60 | ||
getDurationByNumberOfConfirmations(confirmations) * 1.5 |
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.
Is this underlying calc based on the chain being used i.e was it specific to Ethereum, and now needs to be adjusted based on chain being used?
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.
The total time required for the minting process to finalize is at least one hour, consdiering the necessary optimisticMintingDelay
in TBTCVault
contract time to elapse. This is not realistically aligned with how the code was originally implemented, since it was based solely on the duration of Bitcoin network confirmations.
Thus, considering that the finalizeOptimisticMint
function must be called for the deposit to be finalized, it becomes difficult to precisely determine how long the entire process will take. On top of that, this extra waiting period can include an additional 15-30 minutes that may be required for bridging from L1 to L2 in the event of a cross-chain deposit. Consequently, increasing the total estimated time by 50% provides a more flexible estimate, taking into account both the new dynamics introduced by optimistic minting and the 15–30 minutes typically needed for L1–L2 bridging.
const hours = (Math.round(durationInMinutes / 30) * 30) / 60 | ||
getDurationByNumberOfConfirmations(confirmations) * 1.5 | ||
// Round down the minutes to the nearest half-hour | ||
const hours = (Math.floor(durationInMinutes / 30) * 30) / 60 |
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.
Why the rounding down instead of up?
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.
Math.floor is used instead of Math.round to ensure that it always get round down to the nearest 30-minute block avoiding overstating the number of hours.
[Token.Keep]: keep.contract!, | ||
[Token.Nu]: nu.contract!, |
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.
Do we still care about these? Is this related to legacy staking which is no more? (cc @cygnusv )
If so, might be out of scope for this PR, but we should probably remove this.
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 recently caught an user in Discord requesting information to how to upgrade his Nu token to T, so I believe it might still be relevant to keep those functionalities in place for the time being.
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.
Ah yes, the vending machine. I was really thinking with respect to staking.
|
||
const requestEthereumAccount = useCallback(async () => { | ||
// The Goerli testnet become deprecated. However, we did not test Ledger | ||
// Live on Sepolia yet, so we're leaving the Goerli config for now in the | ||
// code. | ||
const currencyId = supportedChainId === "1" ? "ethereum" : "ethereum_goerli" | ||
const currencyId = isTestnetNetwork(defaultOrConnectedChainId) |
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.
Would sepolia be considered isTestnetNetwork
in which case "ethereum_goerli" would be used? Is that intended?
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.
Yes any of the Sepolia networks would return true
in isTestnetNetwork
function validation, this implementation follows the comment previously added about the persistance of the ethereum_goerli
usage while Ledger Live currencyId
hasn't been fully tested in Sepolia network.
// The Goerli testnet become deprecated. However, we did not test Ledger | |
// Live on Sepolia yet, so we're leaving the Goerli config for now in the | |
// code. |
] | ||
``` | ||
|
||
Adding a New Network |
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 documentation.
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.
Thanks, this will definitely be useful for future maintenance, such as adding or removing networks that the app should support
|
||
const MAIN_ALCHEMY_URL = "g.alchemy.com/v2/" | ||
|
||
export const getRpcUrl = (chainId?: number | string) => { |
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 also agree that I don't love how deeply entrenched Alchemy is in the code eg. alchemyName
in the network definition. Down the road if we decide to switch away from Alchemy, it would be tricky to replace with some other RPC service like Infura or dRPC.
I'm not familiar with the code, so just speculating, but perhaps you could have a generic API_KEY
and then an additional API_SERVICE_NAME
variable and a mapping of rpc service name -> rpc config, and the configs have properties/methods to provide/adjust the required names/values/templated strings accordingly. Adding a supported service would involve adding a config entry to the mapping...?
That being said, that can probably be addressed/investigated in a separate PR.
src/pages/Staking/HowItWorks/StakingOverview/AuthorizingApplicationsCard.tsx
Outdated
Show resolved
Hide resolved
1ea5bc9
to
63705c7
Compare
@@ -0,0 +1,1352 @@ | |||
{ | |||
"address": "0xE9Eb81F72cDBE9fE82f61EA527e8a3A39f5B023a", |
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.
Was this registry always hardcoded for the dashboard or are you hardcoding it now? I believe that it is typically obtained from a published npm package - https://www.npmjs.com/package/@nucypher/nucypher-contracts for all TACo domains. Same comment for other TacoRegistry.json
files (cc @manumonti , @theref who are more familiar)
Description
Added multi-network support, including updated environment variables, network-specific explorer links, refactored contract integrations and connector handling, and enhanced local storage and minting flow for multi-network compatibility. Fixed ledger and subgraph issues, and centralized network logic.
Type of change
Notice
Issue (if applicable)
closes #715
closes #743
closes #750
closes #760
closes #764
closes #787
Screenshots (if applicable)