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

Improve contract recompilation #475

Merged
merged 8 commits into from
Dec 6, 2024
Merged

Conversation

Lbqds
Copy link
Member

@Lbqds Lbqds commented Nov 29, 2024

This PR introduces two configs to improve contract recompilation:

  1. skipRecompileIfDeployedOnMainnet: This checks the contract code hash to ensure that if the contract has already been deployed to the mainnet, it will not be recompiled.
  2. skipRecompileContracts: Users can add contracts to this list, and all contracts in this list will not be recompiled.
  3. Removes the use of the deployments file to decide whether to recompile contracts, as it complicates the code.

export async function getContractByCodeHash(
nodeProvider: NodeProvider,
codeHash: HexString
): Promise<HexString | undefined> {
Copy link
Member

Choose a reason for hiding this comment

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

We need to consider 429 errors due to rate limiting, and retry a few times if 429 hits

Copy link
Member

Choose a reason for hiding this comment

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

@Lbqds It seems this one is not resolved yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was resolved in the last commit. Since we don’t want to introduce a retry mechanism in web3, the retry on failure will be done in the cli.


networks: Record<NetworkId, Network<Settings>>

enableDebugMode?: boolean
forceRecompile?: boolean
skipRecompileIfDeployedOnMainnet?: boolean
}

export const DEFAULT_CONFIGURATION_VALUES = {
Copy link
Member

Choose a reason for hiding this comment

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

Should we enable skipRecompileOnDeployment by default?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm a bit hesitant about this because most of the contract compilation occurs during the development phase. Once the contract is deployed to the mainnet, the number of compilations should be much less.

If it's enabled by default, unnecessary checks could also be made on the mainnet during the development phase.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Let's not enable it by default.

sourceInfos: SourceInfo[],
artifactsRootDir: string
): Promise<string[]> {
const nodeProvider = new NodeProvider(mainnetNodeUrl ?? defaultMainnetNodeUrl)
Copy link
Member

Choose a reason for hiding this comment

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

Let's check NetworkID to ensure that it's mainnet. It might be other networks by accident.

sourceInfos: SourceInfo[],
artifactsRootDir: string
): Promise<string[]> {
const nodeProvider = new NodeProvider(mainnetNodeUrl ?? defaultMainnetNodeUrl)
Copy link
Member

Choose a reason for hiding this comment

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

It seems mainetNodeUrl won't be undefined, so this check mainnetNodeUrl ?? defaultMainnetNodeUrl may not work.

Copy link
Member Author

Choose a reason for hiding this comment

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

We support both alephium.config.ts and alephium.config.js configurations. When using alephium.config.ts, the compiler will ensure that the node url is not undefined. But when using alephium.config.js, the node url may be undefined.

Copy link
Member

Choose a reason for hiding this comment

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

okay

@polarker polarker merged commit 5204934 into master Dec 6, 2024
6 of 7 checks passed
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.

2 participants