Migration to add keys to service accounts for testing #6969

janezpodhostnik · 2025-02-04T16:14:09Z

I changed the Add key migration (originally added here: #6926) so that the code can be committed to master, but it is not runnable without changing this constant IAmSureIWantToRunThisMigration. If you change it the test will start failing which would then prevent you from accidentally merging that branch.

see: #6970

codecov-commenter · 2025-02-04T16:18:36Z

Codecov Report

Attention: Patch coverage is 0.29851% with 334 lines in your changes missing coverage. Please review.

Project coverage is 41.09%. Comparing base (f0d7ec9) to head (65c285c).

Files with missing lines	Patch %	Lines
cmd/util/ledger/migrations/add_key_migration.go	0.00%	307 Missing ⚠️
...execution-state-extract/execution_state_extract.go	0.00%	25 Missing ⚠️
cmd/util/cmd/execution-state-extract/cmd.go	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6969      +/-   ##
==========================================
- Coverage   43.29%   41.09%   -2.20%     
==========================================
  Files        1558     2130     +572     
  Lines      139443   186669   +47226     
==========================================
+ Hits        60366    76719   +16353     
- Misses      74026   103524   +29498     
- Partials     5051     6426    +1375

Flag	Coverage Δ
unittests	`41.09% <0.29%> (-2.20%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zhangchiqing · 2025-02-05T19:42:45Z

cmd/util/cmd/execution-state-extract/cmd.go

@@ -422,6 +422,8 @@ func run(*cobra.Command, []string) {
 		var migs []migrations.NamedMigration

 		switch flagMigration {
+		case "add-keys":


Suggested change

case "add-keys":

case "add-migrationnetwork-keys":

maybe making the purpose a bit more clear and specific.

zhangchiqing · 2025-02-05T19:46:42Z

cmd/util/ledger/migrations/add_key_migration.go

+	}
+
+	// add key to node accounts
+	for _, nodeAddress := range mainnetNodeAddresses {


As double protection, I would suggest to use a emptyNodeAddresses here. So in order to use it, both emptyNodeAddresses and IAmSureIWantToRunThisMigration have to be updated. Updating only one would only be a no-op.

Suggested change

for _, nodeAddress := range mainnetNodeAddresses {

for _, nodeAddress := range emptyNodeAddresses {

Do you think these will change over time? wouldn't it be better to just have them prepared here?

I mean we keep the mainnetNodeAddresses variable, but we create another emptyNodeAddresses variable. And by default the emptyNodeAddresses is used, so that this is a double protection.

Then, in order to use it in migration mainnet, we need to do 2 things:

set IAmSureIWantToRunThisMigration to true

replace emptyNodeAddresses with mainnetNodeAddresses here.

And if we forgot to do step 2, the IHaveReplacedEmptyNodeAddressesToMainnetAddresses is still false, and the error will remind us.

And if we updated IHaveReplacedEmptyNodeAddressesToMainnetAddresses to true, but didn't actually do step 2, then it will still be a no-op.

Does it make sense?

what about requiring a specific chainID instead?

I think it's more likely that we accidentally merge code with the updated config so adding a check that we wouldn't need to change would help avoid accidental merges allowing this migration against a main network.

zhangchiqing · 2025-02-05T19:49:59Z

cmd/util/ledger/migrations/add_key_migration.go

+	if !IAmSureIWantToRunThisMigration {
+		panic("Cannot run AddKeyMigration migration")
+	}
+


Better to add another "checkbox" to remind the required step:

Suggested change

if !IHaveReplacedEmptyNodeAddressesToMainnetAddresses {

panic("didn't use the mainnet node addreses")

}

zhangchiqing · 2025-02-05T19:50:10Z

cmd/util/ledger/migrations/add_key_migration.go

+	reporter reporters.ReportWriter
+}
+
+var _ AccountBasedMigration = &AddKeyMigration{}


Suggested change

var _ AccountBasedMigration = &AddKeyMigration{}

var _ AccountBasedMigration = (*AddKeyMigration)(nil)

…ontracts-master

peterargue · 2025-02-05T22:35:25Z

cmd/util/ledger/migrations/add_key_migration.go

+// This migration is not safe to run on actual networks.
+// It is used for getting control over system accounts so that they can be tested
+// in a copied environment. When we need to do this it is best to create a branch
+// where this variable is set to true, and use that temporary branc to run migrations.


Suggested change

// where this variable is set to true, and use that temporary branc to run migrations.

// where this variable is set to true, and use that temporary branch to run migrations.

peterargue · 2025-02-05T23:20:37Z

cmd/util/ledger/migrations/add_key_migration.go

+	}
+
+	// add key to node accounts
+	for _, nodeAddress := range mainnetNodeAddresses {


what about requiring a specific chainID instead?

I think it's more likely that we accidentally merge code with the updated config so adding a check that we wouldn't need to change would help avoid accidental merges allowing this migration against a main network.

…mainnet-add-keys-to-core-contracts-master

…ontracts-master

cmd/util/ledger/migrations/add_key_migration.go

…ontracts-master

janezpodhostnik added 2 commits February 3, 2025 14:37

Migration to add keys to service accounts for testing

0d75df6

make the migration commitable to master

6926298

janezpodhostnik requested review from zhangchiqing, j1010001 and peterargue February 4, 2025 16:14

janezpodhostnik self-assigned this Feb 4, 2025

janezpodhostnik requested a review from a team as a code owner February 4, 2025 16:14

janezpodhostnik mentioned this pull request Feb 4, 2025

[Do Not Merge] Enable add key migration #6970

Draft

zhangchiqing reviewed Feb 5, 2025

View reviewed changes

Merge branch 'master' into janez/migration-mainnet-add-keys-to-core-c…

691c749

…ontracts-master

peterargue approved these changes Feb 5, 2025

View reviewed changes

turbolent and others added 4 commits February 5, 2025 15:33

Merge branch 'bastian/clean-up-migration-flags' into janez/migration-…

5e0e1a2

…mainnet-add-keys-to-core-contracts-master

Merge branch 'master' into janez/migration-mainnet-add-keys-to-core-c…

6ef853e

…ontracts-master

add aditional safeguards

07440bf

fix naming

85464a9

zhangchiqing approved these changes Feb 6, 2025

View reviewed changes

cmd/util/ledger/migrations/add_key_migration.go Outdated Show resolved Hide resolved

fix comment

6dae416

janezpodhostnik enabled auto-merge February 6, 2025 16:57

janezpodhostnik added this pull request to the merge queue Feb 6, 2025

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 6, 2025

Merge branch 'master' into janez/migration-mainnet-add-keys-to-core-c…

65c285c

…ontracts-master

j1010001 enabled auto-merge February 6, 2025 19:00

j1010001 added this pull request to the merge queue Feb 6, 2025

Merged via the queue into master with commit 4ce667d Feb 6, 2025
56 checks passed

j1010001 deleted the janez/migration-mainnet-add-keys-to-core-contracts-master branch February 6, 2025 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migration to add keys to service accounts for testing #6969

Migration to add keys to service accounts for testing #6969

janezpodhostnik commented Feb 4, 2025 •

edited

Loading

codecov-commenter commented Feb 4, 2025 •

edited

Loading

zhangchiqing Feb 5, 2025

zhangchiqing Feb 5, 2025

janezpodhostnik Feb 5, 2025

zhangchiqing Feb 5, 2025 •

edited

Loading

peterargue Feb 5, 2025

zhangchiqing Feb 5, 2025

zhangchiqing Feb 5, 2025

peterargue Feb 5, 2025

peterargue Feb 5, 2025

	for _, nodeAddress := range mainnetNodeAddresses {
	for _, nodeAddress := range emptyNodeAddresses {

+	if !IHaveReplacedEmptyNodeAddressesToMainnetAddresses {
+		panic("didn't use the mainnet node addreses")
+	}

	var _ AccountBasedMigration = &AddKeyMigration{}
	var _ AccountBasedMigration = (*AddKeyMigration)(nil)

	// where this variable is set to true, and use that temporary branc to run migrations.
	// where this variable is set to true, and use that temporary branch to run migrations.

Migration to add keys to service accounts for testing #6969

Migration to add keys to service accounts for testing #6969

Conversation

janezpodhostnik commented Feb 4, 2025 • edited Loading

codecov-commenter commented Feb 4, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangchiqing Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janezpodhostnik commented Feb 4, 2025 •

edited

Loading

codecov-commenter commented Feb 4, 2025 •

edited

Loading

zhangchiqing Feb 5, 2025 •

edited

Loading