-
Notifications
You must be signed in to change notification settings - Fork 376
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
base: release/core-contracts/12
Are you sure you want to change the base?
Conversation
This reverts commit 2723b3a.
cb9d5fa
to
16eca70
Compare
packages/protocol/scripts/foundry/create_and_migrate_anvil_devchain.sh
Outdated
Show resolved
Hide resolved
…norepo into martinvol/fixSupplyCelo
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.
Left some style and nit comments. I'll approve when CI is green, and we tested the bug @shazarre logged:
packages/protocol/scripts/foundry/create_and_migrate_anvil_devchain.sh
Outdated
Show resolved
Hide resolved
packages/protocol/scripts/foundry/create_and_migrate_anvil_devchain.sh
Outdated
Show resolved
Hide resolved
packages/protocol/scripts/foundry/create_and_migrate_anvil_devchain.sh
Outdated
Show resolved
Hide resolved
packages/protocol/scripts/foundry/create_and_migrate_anvil_devchain.sh
Outdated
Show resolved
Hide resolved
Co-authored-by: Arthur Gousset <46296830+arthurgousset@users.noreply.github.com>
…chain.sh Co-authored-by: Arthur Gousset <46296830+arthurgousset@users.noreply.github.com>
…norepo into martinvol/fixSupplyCelo
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 |
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.
I couldn't figure out how to tell anvil how many addresses to fund, so hardcoded the 9
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.
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:
Linked the issue this is related to in PR description and posted a comment in the issue so @shazarre gets an update: |
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 |
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.
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:
celo-monorepo/packages/protocol/scripts/foundry/create_and_migrate_anvil_devchain.sh
Line 31 in 965bb57
forge build --contracts $PWD/test-sol/devchain |
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 |
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.
Released a test version of the devchain to NPM using the protocol-devchain-anvil workflow trigger (see screenshot):
I suggest this PR gets merged if and when @shazarre confirmed the bug is fixed on his side using the test version.
This PR is stale and will be closed in 30 days without activity |
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.