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

Allow export of global registers (owner is empty string) #5939

Merged
merged 7 commits into from
May 20, 2024

Conversation

turbolent
Copy link
Member

Global registers have an empty address.

Allow the util commands which allow filtering of state, execution-state-extract and extract-payloads-by-address, to allow such global registers to be included in the final payloads.

@turbolent turbolent requested review from a team May 16, 2024 18:36
@codecov-commenter
Copy link

codecov-commenter commented May 16, 2024

Codecov Report

Attention: Patch coverage is 91.89189% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 55.81%. Comparing base (fc3e499) to head (0491083).
Report is 37 commits behind head on master.

Files Patch % Lines
cmd/util/cmd/execution-state-extract/cmd.go 80.00% 1 Missing and 1 partial ⚠️
cmd/util/ledger/util/payload_file.go 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5939      +/-   ##
==========================================
+ Coverage   55.61%   55.81%   +0.20%     
==========================================
  Files        1128     1128              
  Lines       88939    88963      +24     
==========================================
+ Hits        49459    49655     +196     
+ Misses      34767    34569     -198     
- Partials     4713     4739      +26     
Flag Coverage Δ
unittests 55.81% <91.89%> (+0.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.

@@ -239,8 +238,8 @@ func writeSelectedPayloads(logger zerolog.Logger, w io.Writer, payloads []*ledge
return includedPayloadCount, nil
}

func includePayloadByAddresses(payload *ledger.Payload, addresses []common.Address) (bool, error) {
if len(addresses) == 0 {
func includePayloadByOwners(payload *ledger.Payload, owners map[string]struct{}) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be better to just always include the system registers, because it doesn't really make sense not to. They are small, so if you want to do further migrations with the resulting data they wont affect much and you basically need them to properly use the resulting state in transactions and scripts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was thinking of doing that, thanks for seconding it. I was not aware of the empty registers and given we only test/develop with filtered state locally, I accidentally broke the migration. Will make the appropriate change

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in a82f463

@turbolent turbolent requested review from janezpodhostnik and a team May 17, 2024 17:09
@turbolent turbolent enabled auto-merge May 18, 2024 00:30
@turbolent turbolent added this pull request to the merge queue May 20, 2024
Merged via the queue into master with commit 0436edf May 20, 2024
55 checks passed
@turbolent turbolent deleted the bastian/allow-export-of-global-registers branch May 20, 2024 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants