-
Notifications
You must be signed in to change notification settings - Fork 179
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
Deduplicate contract names migration #5143
Conversation
cmd/util/ledger/migrations/deduplicate_contract_names_migration.go
Outdated
Show resolved
Hide resolved
cmd/util/ledger/migrations/deduplicate_contract_names_migration_test.go
Outdated
Show resolved
Hide resolved
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 comments I think you should address
cmd/util/ledger/migrations/deduplicate_contract_names_migration.go
Outdated
Show resolved
Hide resolved
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.
Nice! I left some suggestions that can reduce memory use, number of allocations, and also tried to speedup large account processing by avoiding snapshot.
cmd/util/ledger/migrations/deduplicate_contract_names_migration.go
Outdated
Show resolved
Hide resolved
cmd/util/ledger/migrations/deduplicate_contract_names_migration.go
Outdated
Show resolved
Hide resolved
cmd/util/ledger/migrations/deduplicate_contract_names_migration.go
Outdated
Show resolved
Hide resolved
6b4c3ca
to
2faa388
Compare
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.
Nice! I left a couple suggestions.
Also maybe add more tests (some ideas):
- without contract name payload
- with one contract name
- with two unique contract names
- with two duplicate contact names
- with a list of randomized contract names (with duplicates)
cmd/util/ledger/migrations/deduplicate_contract_names_migration.go
Outdated
Show resolved
Hide resolved
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.
Nice! Thanks for adding more tests. 👍 Only new suggestion is to maybe remove empty contract name payload.
Add migration to deduplicate account contracts names.
extracted from: #4633
ref: #3825