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

Fix supply of Celo in Anvil migrations #11176

Open
wants to merge 15 commits into
base: release/core-contracts/12
Choose a base branch
from

Conversation

martinvol
Copy link
Contributor

@martinvol martinvol commented Jul 26, 2024

Description

Set the supply to what the known addresses have. This can only be done after the migrations as the Celo token doesn't exist then. This can not be done from the migration script as it requires interpersonating.

Other changes

None

Tested

In migration tests.

Related issues

Backwards compatibility

Actually fixes it.

Documentation

it is a expected behaviour.

@martinvol martinvol requested a review from a team as a code owner July 26, 2024 11:55
@martinvol martinvol force-pushed the martinvol/fixSupplyCelo branch from cb9d5fa to 16eca70 Compare July 26, 2024 12:03
Copy link
Contributor

@arthurgousset arthurgousset left a comment

Choose a reason for hiding this comment

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

Left some style and nit comments. I'll approve when CI is green, and we tested the bug @shazarre logged:

martinvol and others added 6 commits July 26, 2024 15:20
Co-authored-by: Arthur Gousset <46296830+arthurgousset@users.noreply.github.com>
…chain.sh

Co-authored-by: Arthur Gousset <46296830+arthurgousset@users.noreply.github.com>
cast rpc anvil_setBalance $CELO_VM_ADDRESS $ETHER_IN_WEI --rpc-url $ANVIL_RPC_URL

# increase the supply (it is harcoded as bash overflows)
# ideally this number should come from the amount of address funded when Anvil loads
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't figure out how to tell anvil how many addresses to fund, so hardcoded the 9

Copy link
Contributor

@arthurgousset arthurgousset Jul 30, 2024

Choose a reason for hiding this comment

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

I think this is a known limitation. I looked into this in the past and couldn't find an easy way to access the number of default accounts either:

@arthurgousset
Copy link
Contributor

arthurgousset commented Jul 30, 2024

Linked the issue this is related to in PR description and posted a comment in the issue so @shazarre gets an update:

@arthurgousset arthurgousset self-requested a review July 30, 2024 15:45
Comment on lines 9 to +14
source $PWD/scripts/foundry/create_and_migrate_anvil_devchain.sh

# Before running the migration script, check that the tests compile
# This will avoid having to wait for the whole chain to start just to realize
# there's a compilation error at the very end
forge build --contracts $PWD/test-sol/integration
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Explicitly building the contracts here might not be necessary in this script since it runs create_and_migrate_anvil_devchain.sh which has the same call:

forge build --contracts $PWD/test-sol/devchain

Suggested change
source $PWD/scripts/foundry/create_and_migrate_anvil_devchain.sh
# Before running the migration script, check that the tests compile
# This will avoid having to wait for the whole chain to start just to realize
# there's a compilation error at the very end
forge build --contracts $PWD/test-sol/integration
source $PWD/scripts/foundry/create_and_migrate_anvil_devchain.sh

Copy link
Contributor

@arthurgousset arthurgousset left a comment

Choose a reason for hiding this comment

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

Released a test version of the devchain to NPM using the protocol-devchain-anvil workflow trigger (see screenshot):

image

I suggest this PR gets merged if and when @shazarre confirmed the bug is fixed on his side using the test version.

Copy link
Contributor

This PR is stale and will be closed in 30 days without activity

@github-actions github-actions bot added the stale label Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants