Skip to content
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: verify contracts on filfox #151

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

deanamiel
Copy link
Contributor

@deanamiel deanamiel commented Jan 12, 2024

Script to verify contracts on filfox explorer as per AXE-2937

@deanamiel deanamiel requested a review from a team as a code owner January 12, 2024 10:16
const { addBaseOptions } = require('./cli-utils');
const { validateParameters, loadConfig, printInfo, printError } = require('./utils');

async function verifyFilfox(options) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be integrated into our normal verifyContract method. If chain is filecoin then call this helper method with appropriate data. you can look into how the existing verify method extracts the compiler version/optimizer runs to do the same (maybe even import that method if possible from hardhat-verify)

},
};

const optimizerDetails = '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are the optimizerDetails and some other fields empty? Our current config has the optimizer details. Are these fields not necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree, was having some trouble importing the configurations from the hardhat config in other repositories

Comment on lines +25 to +26
"path": "^0.12.7",
"axios": "^1.6.2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these aren't dev dependencies since it's for an exported function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I didn't have them as dependencies axelar chains config tests were failing, should I move them from dev dependencies into normal dependencies?


try {
const buildInfoPath = path.join(dir, 'build-info');
const smallestFile = await getSmallestFile(buildInfoPath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the logic behind getting the smallest one? non-obvious behaviour should be commented

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that there are often multiple build files produced within the build-info folder, some are extremely large and unfeasible to parse just for compiler/optimize information but all seem to contain the same config information which is why I added a function to just parse the smallest file present. I agree it is a bit convoluted but I'm not sure how else to go about it


let buildInfo;

try {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move try/catch to be specifically over the code that can fail


const config = require(`../../info/${env}.json`);
const contractConfig = config.chains.filecoin.contracts[contractName];
const contractFileName = getContractFileName(contractConfig, contract);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't contractName already being passed in? you can enforce it to be passed in, and update usages in our code instead of the limited reverse map

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The config map has different names for addresses than actual contract file names, so this should fail @deanamiel.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@milapsheth , @deanamiel can we modify all verifyContract() calls withinverify-contract.js to have a hardcoded contract name with it, which will simplify script execution for all the contracts. I will also be able to use this in my PR: #162

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contractName being passed would just be InterchainTokenService whereas I need each individual contract name within ITS to find the correct contract path to the flattened source code. @blockchainguyy yeah that is true, it will not work for a few contract names, I thought of adding an optional argument for the hardcoded contract name to verifyContract() as well but am not sure it's worth all these changes for only filfox verification for their specific verification endpoint requirements, thoughts @milapsheth ?

metadata: '',
};

const api = config.chains.filecoin.explorer?.api;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get the API from hardhat network or options.api?


if (fileOrDirectory.isDirectory()) {
directoriesToSearch.push(fullPath);
} else if (fileOrDirectory.isFile() && fileOrDirectory.name === targetFileName) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: if we convert fileOrDirectory.name and targetFileName to lowercase and compare them we should be able to simplify getContractFileName by removing slicing and first letter conversion to uppercase, which should increase code readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants