-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat(svm): include solana artifacts in package build output #806
base: master
Are you sure you want to change the base?
feat(svm): include solana artifacts in package build output #806
Conversation
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
printf toupper(substr($i,1,1)) tolower(substr($i,2)); | ||
} | ||
}') | ||
newName="${camelCaseName}Anchor" |
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.
Prepend "Anchor" to the types to prevent conflicts with evm types
printf toupper(substr($i,1,1)) tolower(substr($i,2)); | ||
} | ||
}') | ||
IMPORTS="${IMPORTS}const ${camelCaseName}Idl = require(\"./${filename}\");\n" |
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.
We use require()
for the IDLs similarly as before in the scripts to avoid ts issues:
const svmSpokeIdl = require("../../target/idl/svm_spoke.json"); |
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
@@ -1,3 +1,4 @@ | |||
export * from "./typechain"; | |||
export * from "./src/DeploymentUtils"; | |||
export * from "./utils"; | |||
export * from "./src/svm"; |
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 program getters, types, and IDLs can be imported directly from @across-protocol/contracts
, as well as utility functions (we should move the latter to the SDK next).
d3a8991
to
97973eb
Compare
} from "./assets"; | ||
|
||
export function getConnectedProgram<P extends Idl>(idl: P, provider: Provider) { | ||
return new Program<P>(idl, provider); |
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.
Note: In the SVM scripts, we have been connecting to the program ID defined in the IDL file. Since these IDs are generated automatically, any future changes to the program, if merged into master, could cause the program ID to drift.
We should explore alternatives where the caller can pass a specific program ID in a follow-up PR so we can use this in our bots to prevent unexpected programIDs changes. Wdyt?
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.
agree, we should better implement swapping out the programId
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.
Finally implemented the programId override as it enhances the usability of these functions
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.
ye, there are similar things we'd want to do on the evm side I think wherein the deployments/release process also includes a hash or some deterministic way to derive the code we are revering to on a release.
can fit into this as well.
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 much better that we had before!
136d7d2
to
10a73f9
Compare
export function getDeployedAddress(contractName: string, networkId: number, throwOnError = true): string | undefined { | ||
export function getDeployedAddress( | ||
contractName: string, | ||
networkId: 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.
Network id 1810017368444177321
(solana-devnet) overflows js numbers so we need to pass it as a string
518bf52
to
b766c81
Compare
Signed-off-by: Pablo Maldonado <[email protected]>
b766c81
to
804fa7a
Compare
@@ -26,10 +26,11 @@ | |||
"lint-fix": "yarn prettier --write **/*.js **/*.ts ./programs/**/*.rs ./contracts**/*.sol && cargo +nightly fmt --all && cargo clippy", | |||
"clean-fast": "for dir in node_modules cache cache-zk artifacts artifacts-zk dist typechain; do mv \"${dir}\" \"_${dir}\"; rm -rf \"_${dir}\" &; done", | |||
"clean": "rm -rf node_modules cache cache-zk artifacts artifacts-zk dist typechain", | |||
"generate-svm-assets": "sh ./scripts/generate-svm-assets.sh", |
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.
we should think about making this file easier to reason about with this new command you've added. the equivalent evm command is generate-contract-types
. I tried with the other commands to make it clear what they are (solidity/js/rust/evm/svm) but this one is now a bit inconsistent. perhaps if we call it generate-svm-assets
and then re-name the generate-contract-types
to generate-evm-assets
?
also, I think that given this command is not yet within the prepublish
command we could actually be safe to merge this into master without it joining the released package.
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.
Updated ✅
also, I think that given this command is not yet within the prepublish command we could actually be safe to
merge this into master without it joining the released package.
I think it is, its part of the build as we need to run yarn generate-svm-assets
before yarn build-ts
. See this:
Line 33 in 907b7d7
"build": "yarn build-evm && yarn build-svm && yarn generate-svm-assets && yarn build-ts", |
Let's wait for @mrice32 & @nicholaspai to see what they think the proposed changes.
Signed-off-by: Pablo Maldonado <[email protected]>
…-is-there-a-type-safe-interface
@@ -163,5 +163,10 @@ | |||
"SpokePool": { "address": "0xeF684C38F94F48775959ECf2012D7E864ffb9dd4", "blockNumber": 1139240 }, | |||
"SpokePoolVerifier": { "address": "0xB4A8d45647445EA9FC3E1058096142390683dBC2", "blockNumber": 1152853 }, | |||
"MulticallHandler": { "address": "0x924a9f036260DdD5808007E1AA95f08eD08aA569", "blockNumber": 1145284 } | |||
}, | |||
"1810017368444177321": { |
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 our chain id representation of solana-devnet, see this:
Lines 8 to 12 in ddbe2fc
export const getSolanaChainId = (cluster: "devnet" | "mainnet"): BigNumber => { | |
return BigNumber.from( | |
BigInt(ethers.utils.keccak256(ethers.utils.toUtf8Bytes(`solana-${cluster}`))) & BigInt("0xFFFFFFFFFFFFFFFF") | |
); | |
}; |
…-is-there-a-type-safe-interface
Signed-off-by: Pablo Maldonado <[email protected]>
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is a critical CVE?Contains a Critical Common Vulnerability and Exposure (CVE). Remove or replace dependencies that include known critical CVEs. Consumers can use dependency overrides or npm audit fix --force to remove vulnerable dependencies. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Report too large to display inline |
Changes proposed in this PR: