-
Notifications
You must be signed in to change notification settings - Fork 71
feat(Foundry): Updated constants usage #1111
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
@@ -1,11 +1,7 @@ | |||
// SPDX-License-Identifier: BUSL-1.1 |
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.
Most of the changes in this file is just updating the JSON keys to uppercase
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 - this is going to be really useful. I have a couple of broader comments but I like the direction this is going.
// Get OP Stack addresses for this chain and Optimism | ||
uint256 optimismChainId = getChainId("OPTIMISM"); |
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.
Only tangentially related to this change, but I feel like we should be reading the spoke chain ID from the environment and then using it to resolve all the correct addresses. With that approach I think we we remove a barrier to consolidating a lot of scripts. wdyt?
(This approach worked well for the OP Adapter deployment script here)
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.
Thats good idea, I was thinking of using this strategy in the spokepool consolidation pr, wanted to get some initial feedback on the direction first before I put any more effort
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.
Yeah I left some comments on that one; we have an unresolved question about the exact strategy to take, but in general it's unavoiable that we consolidate these contracts & scripts so I'm fully in favour of frontloading as much of that as possible, including setting SPOKE_CHAIN_ID in the env and auto-configuring everything based on that.
@@ -180,19 +180,21 @@ contract DeploymentUtils is Script, Test, Constants, DeployedAddresses { | |||
*/ | |||
function isTestnet(uint256 chainId) internal view returns (bool) { |
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.
Testnet chain IDs are identified separately via TESTNET_CHAIN_IDs
in constants. Could we check for chainId
inclusion in that mapping instead of enumerating these here separately?
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.
done 45a034a
script/utils/constants.json
Outdated
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.
Given that this is generated, wdyt about storing it separately from code? i.e. under an artefacts
directory or something?
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.
My concern with this is that the file can get out of date. From what I can tell, foundry doesn't have a like a pre-script hook where we can generate this file.
I think in general better approach would be to have this file generated in the constants repo and we just import it here
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.
Yeah; I like proding it as a build artefact over there and importing it here.
Given the above it's a moot point, but I don't fwiw understand what you mean about the file getting out of date; I'm just suggesting that it be stored separately from code in the repo.
if (cctpDomain == -1) { | ||
revert("Circle domain ID not found"); | ||
} |
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.
Does this impliy that we know CCTP is supported for the chain in advance; should we have a separate function that determines whether it's supported? Or should the revert be caught to infer that CCTP is not supported?
Background: For a generic OP adapter that supports all variety of configurations, we basically need to configure for native or bridged USDC dynamically based on the domain ID.
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.
my idea here was that we dont want to have a script accidentally set -1
as domain id but I think that would fail because of uint256
on the contract.
added hasCctpDomain
that can be used upstream 9daffac
script/utils/DeploymentUtils.sol
Outdated
@@ -201,10 +203,7 @@ contract DeploymentUtils is Script, Test, Constants, DeployedAddresses { | |||
* @param chainId Chain ID to check | |||
*/ | |||
function checkZkSyncChain(uint256 chainId) internal view { |
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.
Should we call this isChainZkStack
? (as we're checking for ZK_STACK
family here)
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.
done 7b16cb5
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 took a while for me to figure this out, but Matter Labs call it Elastic Chain. ZK_STACK is unfortunately a pretty heavily overloaded term so I am wondering whether we should just rename it everywhere.
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.
Looks good. It's be nice to have https://github.com/across-protocol/contracts/pull/1111/files#r2356459637 before we merge if possible
script/utils/Constants.sol
Outdated
|
||
function getChainFamily(uint256 chainId) public view returns (string memory) { | ||
uint256 family = vm.parseJsonUint(file, string.concat(".PUBLIC_NETWORKS.", vm.toString(chainId), ".family")); | ||
return vm.parseJsonUint(file, string.concat(".CHAIN_FAMILIES.", vm.toString(family))); |
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 gives me LSP errors. Seems like we want to return a string
, but returning a uint?
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.
done 7b16cb5
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
98330d4
to
07a4bb2
Compare
Signed-off-by: Faisal Usmani <[email protected]>
With the new constants JSON file from #1105, need to update its usage in deploy scripts