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

Add auto-gzip contracts in cli when uploading #19

Merged
merged 3 commits into from
Jan 14, 2020

Conversation

anilcse
Copy link
Contributor

@anilcse anilcse commented Jan 11, 2020

Fixes: #18

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md

  • Reviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge PR #XYZ: [title]" (coding standards)

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks good.
The utils could be simplified a bit.
And it seems like the ci is demanding a test case for utils.go
Otherwise, nice work. Happy to merge when cleaned up

return false
}

in := io.LimitReader(bytes.NewReader(input), maxSize)
Copy link
Member

Choose a reason for hiding this comment

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

Alex does this to maintain a stream for his reader functionality. You can just do a bytes.Equal(input[:3], gzipIdent)

The function is correct and nice code organization btw. Just could be simpler

Copy link
Member

Choose a reason for hiding this comment

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

Oh, and compare on maxSize as well I guess

// IsWasm checks if the file contents are of wasm binary
func IsWasm(input []byte) bool {
if len(input) < 3 {
return false
Copy link
Member

Choose a reason for hiding this comment

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

Same as above. Function looks good, but could be simpler.


// GzipIt compresses the input ([]byte)
func GzipIt(input []byte) ([]byte, error) {
// Create gzip writer.
Copy link
Member

Choose a reason for hiding this comment

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

Nice

@@ -57,6 +59,17 @@ func StoreCodeCmd(cdc *codec.Codec) *cobra.Command {
return err
}

// gzip the wasm file
if wasmUtils.IsWasm(wasm) {
wasm, err = wasmUtils.GzipIt(wasm)
Copy link
Member

Choose a reason for hiding this comment

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

Nice

@anilcse
Copy link
Contributor Author

anilcse commented Jan 14, 2020

@ethanfrey updated all the changes. It is r4r

@codecov-io
Copy link

codecov-io commented Jan 14, 2020

Codecov Report

Merging #19 into master will increase coverage by 0.18%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #19      +/-   ##
==========================================
+ Coverage    59.2%   59.38%   +0.18%     
==========================================
  Files          11       12       +1     
  Lines         929      943      +14     
==========================================
+ Hits          550      560      +10     
- Misses        329      331       +2     
- Partials       50       52       +2
Impacted Files Coverage Δ
x/wasm/client/utils/utils.go 71.42% <71.42%> (ø)

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 d12c434...10e49dc. Read the comment docs.

@anilcse anilcse requested a review from ethanfrey January 14, 2020 04:38
@anilcse anilcse marked this pull request as ready for review January 14, 2020 04:38
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for cleanup and adding tests

}

// IsWasm checks if the file contents are of wasm binary
func IsWasm(input []byte) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the cleanup, so simple

@ethanfrey ethanfrey merged commit d365da2 into master Jan 14, 2020
@ethanfrey ethanfrey deleted the vw/auto-gzip-contracts branch January 14, 2020 11:16
zemyblue referenced this pull request in Finschia/wasmd Jan 2, 2023
loloicci pushed a commit to loloicci/wasmd that referenced this pull request Apr 19, 2023
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.

Auto-gzip contracts in cli when uploading
3 participants