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

agreement: add omitempty to MessageHandle in agreement message #5593

Merged
merged 3 commits into from
Jul 28, 2023

Conversation

iansuvak
Copy link
Contributor

@iansuvak iansuvak commented Jul 21, 2023

Summary

As listed in comments and in the issue below we want to remove no longer necessary field from the struct. We will still need another PR and another consensus release before it's safe to delete but we need to add omitempty first directive to the field only which this PR does. Not including omitempty was an overlook on the original PR was an overlook and I didn't know that the directive could be applied to a single field. Only noticed it recently while working on other unmarshalling msgp code.

Once the consensus release is out we can actually delete the field and the associated tests as it will be guaranteed that no nodes running agreement will have this field encoded in their crashDB.

Test Plan

In addition to modifying the existing test below, also ran across 364 rows obtained from a testnet node instrumented to not overwrite the data row and instead append to it. It successfully decoded all of the rows and re-encoded them without including references to MessageHandle. For more details about the test including a screenshot see the original PR at #4644

@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #5593 (25a4a97) into master (d316914) will increase coverage by 0.00%.
Report is 23 commits behind head on master.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5593   +/-   ##
=======================================
  Coverage   55.79%   55.79%           
=======================================
  Files         446      446           
  Lines       63418    63418           
=======================================
+ Hits        35381    35384    +3     
+ Misses      25668    25667    -1     
+ Partials     2369     2367    -2     
Files Changed Coverage Δ
agreement/message.go 71.42% <ø> (ø)

... and 11 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@iansuvak iansuvak self-assigned this Jul 21, 2023
@iansuvak iansuvak marked this pull request as draft July 21, 2023 18:38
@iansuvak iansuvak added the tech debt Things that need re-work for simplification / sanitization to reduce implementation overhead label Jul 25, 2023
@iansuvak iansuvak marked this pull request as ready for review July 25, 2023 21:17
@iansuvak iansuvak requested review from cce and algorandskiy July 25, 2023 21:17
@algorandskiy algorandskiy changed the title msgp: add omitempty to MessageHandle in agreement message agreement: add omitempty to MessageHandle in agreement message Jul 28, 2023
@algorandskiy algorandskiy merged commit fd15f4e into algorand:master Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip-Release-Notes Team Carbon-11 tech debt Things that need re-work for simplification / sanitization to reduce implementation overhead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants