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.

Sign up for GitHub

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

Jump to bottom

chore (pkg/scale) integrate scale into dot/network and dot/digest #1685

Closed
wants to merge 25 commits into from
Closed

chore (pkg/scale) integrate scale into dot/network and dot/digest #1685

wants to merge 25 commits into from

Conversation

jimjbrettj
Copy link
Contributor

Changes

  • Integrate scale into network and digest packages
  • integrate scale into network digest type

Tests

go test ./dot/network -short -v
go test ./dot/digest -v
go test ./dot/types -v

Issues

@codecov
Copy link

codecov bot commented Jul 8, 2021 •
edited
Loading

Codecov Report

Merging #1685 (772bb8f) into development (c981d2e) will decrease coverage by 0.02%.
The diff coverage is 83.67%.

❗ Current head 772bb8f differs from pull request most recent head 14be893. Consider uploading reports for the commit 14be893 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1685      +/-   ##
===============================================
- Coverage        58.92%   58.89%   -0.03%     
===============================================
  Files              182      182              
  Lines            18615    18589      -26     
===============================================
- Hits             10968    10948      -20     
  Misses            5737     5737              
+ Partials          1910     1904       -6
Flag Coverage Δ
unit-tests 58.89% <83.67%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
dot/types/consensus_digest.go 0.00% <0.00%> (ø)
dot/digest/digest.go 64.57% <100.00%> (-0.93%) ⬇️
dot/network/block_announce.go 72.61% <100.00%> (-1.88%) ⬇️
dot/network/light.go 87.67% <100.00%> (-1.36%) ⬇️
dot/network/message.go 70.00% <100.00%> (+0.18%) ⬆️
dot/network/test_helpers.go 81.91% <100.00%> (ø)
dot/network/transaction.go 48.83% <100.00%> (+2.49%) ⬆️
lib/babe/epoch.go 64.51% <0.00%> (-2.16%) ⬇️
lib/blocktree/blocktree.go 57.05% <0.00%> (-1.18%) ⬇️
lib/grandpa/grandpa.go 60.06% <0.00%> (-0.62%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c981d2e...14be893. Read the comment docs.

@jimjbrettj jimjbrettj marked this pull request as ready for review July 14, 2021 17:22
@jimjbrettj
Copy link
Contributor Author

Still trying to get docker stress test to pass, but besides that should be ready for review

noot
noot reviewed Jul 14, 2021
View reviewed changes
dot/network/light.go
@@ -40,22 +40,17 @@ func (l *LightRequest) SubProtocol() string {
}

// Encode encodes a LightRequest message using SCALE and appends the type byte to the start
// TODO Remove encode/decode receiver functions
Copy link
Contributor

Choose a reason for hiding this comment

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

woudl this require updating the Message interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I believe so, I don't have to change it if I shouldn't. Was just thinking less receivers the better

noot
noot reviewed Jul 14, 2021
View reviewed changes
dot/network/message_test.go Outdated
// ParentHash: Hash: 0x4545454545454545454545454545454545454545454545454545454545454545
// Number: *big.Int // block number: 1
// StateRoot: Hash: 0xb3266de137d20a5d0ff3a6401eb57127525fd9b2693701f0bf5a8a853fa3ebe0
// ExtrinsicsRoot: Hash: 0x03170a2e7597b7b7e3d84c05391d139a62b157e78786d8c082f29dcf4c111314
// Digest: []byte

// mtparenthash bnstateroot extrinsicsroot di
expected, err := common.HexToBytes("0x454545454545454545454545454545454545454545454545454545454545454504b3266de137d20a5d0ff3a6401eb57127525fd9b2693701f0bf5a8a853fa3ebe003170a2e7597b7b7e3d84c05391d139a62b157e78786d8c082f29dcf4c1113140000")
expected, err := common.HexToBytes("0x01454545454545454545454545454545454545454545454545454545454545454504b3266de137d20a5d0ff3a6401eb57127525fd9b2693701f0bf5a8a853fa3ebe003170a2e7597b7b7e3d84c05391d139a62b157e78786d8c082f29dcf4c1113140000")
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this changed? it should be the same as before (in fact it must be, or interop will break)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it is because it was encoded as an optional so had a leading byte appended. Will look into working around this

noot
noot suggested changes Jul 14, 2021
View reviewed changes
Copy link
Contributor

@noot noot left a comment

Choose a reason for hiding this comment

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

the encoding of the BlockAnnounceMessage or any message type shouldn't change, can you update it to be the same as before? likely why the stress tests are failing also

@jimjbrettj jimjbrettj marked this pull request as draft July 15, 2021 16:50
@jimjbrettj jimjbrettj closed this Jul 22, 2021
@jimjbrettj jimjbrettj deleted the jimmy/integrateScaleNetwork branch October 28, 2021 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noot noot noot requested changes

@arijitAD arijitAD Awaiting requested review from arijitAD

@EclesioMeloJunior EclesioMeloJunior Awaiting requested review from EclesioMeloJunior EclesioMeloJunior is a code owner

@edwardmack edwardmack Awaiting requested review from edwardmack

@timwu20 timwu20 Awaiting requested review from timwu20 timwu20 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jimjbrettj @noot