Bootstrap with EVM by default #5482
Conversation
janezpodhostnik
force-pushed
the
janez/bootstrap-with-evm-as-default
branch
from
3bbd0cc to
e949a4e
Compare
| b.registerNodes(service, fungibleToken, flowToken) | ||
|
|
||
| // set the list of nodes which are allowed to stake in this network | ||
| b.setStakingAllowlist(service, b.identities.NodeIDs()) | ||
|
|
||
| // sets up the EVM environment | ||
| b.setupEVM(service, fungibleToken, flowToken) |
There was a problem hiding this comment.
Ah, so this is why the indices were off last time. We set up the EVM after creating the node accounts 💡
There was a problem hiding this comment.
Yes! I did not see that registerNodes creates new accounts.
There was a problem hiding this comment.
Maybe leave a warning in the code to make it more obvious that registerNodes creates accounts (maybe even improve naming) and maybe that contracts should be deployed before it.
| FlowTokenAccountIndex = 3 | ||
| FlowFeesAccountIndex = 4 | ||
| EVMStorageAccountIndex = 5 | ||
|
|
||
| LastSystemAccountIndex = EVMStorageAccountIndex |
There was a problem hiding this comment.
I suggest adding additional documentation here.
These are the account indexes of system contracts as deployed by the default bootstrapping.
However, this package also contains information about chains which were not deployed by the default bootstrapping, like Mainnet and Testnet. For those chains, I don't think these indices will point to the right contracts.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/stable-cadence #5482 +/- ##
==========================================================
- Coverage 56.01% 55.98% -0.04%
==========================================================
Files 1042 1042
Lines 101950 101982 +32
==========================================================
- Hits 57112 57096 -16
- Misses 40491 40533 +42
- Partials 4347 4353 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -62,7 +64,7 @@ func TestWriteMachineAccountFiles(t *testing.T) { | |||
| expected := make(map[string]bootstrap.NodeMachineAccountInfo) | |||
| for i, node := range nodes { | |||
| // See comments in WriteMachineAccountFiles for why addresses take this form | |||
| addr, err := chain.AddressAtIndex(uint64(6 + i*2)) | |||
| addr, err := chain.AddressAtIndex(uint64(systemcontracts.LastSystemAccountIndex + (i+1)*2)) | |||
There was a problem hiding this comment.
@jordanschalm can you confirm this logic is correct?
janezpodhostnik
merged commit 524be1e
into
feature/stable-cadence
|
@janezpodhostnik The merge of this PR broke the tests on the target branch, see https://github.com/onflow/flow-go/actions/runs/8116077900/job/22185298828. Can you please have a look? |
closes: #5491 and #5382
Should be ported back to master as well