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

cw1-subkeys-ng: Additional follow up improvements #506

Merged
merged 4 commits into from
Oct 25, 2021

Conversation

hashedone
Copy link
Contributor

Additional upgrades of cw1-subkeys-ng contracts.

Mosltly naming, but also hiding quriers behind feature flag so they are not generated in wasm binary.

ethanfrey
ethanfrey previously approved these changes Oct 22, 2021
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.

Nice cleanup

@@ -3,11 +3,12 @@ pub mod error;
pub mod interfaces;
pub mod msg;
pub mod multitest;
#[cfg(any(test, feature = "querier"))]
Copy link
Member

Choose a reason for hiding this comment

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

did this have any effect?
I assume it would have been optimised away, but curious if this affects was size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may not be it is to be chcecked. The reason why they may not be optimized is, that they are public and visible from this point, and our wasm binaries are actually wasm libraries, so I am pretty sure optimizer has no rights to cut them off. I think, that in librray builds any dispatcher would not occur in the library, because they are pub(crate), and if I don't generate neither entry points nor multitest helpers they are straightly deadcode, so cut off, but not in this case.

maurolacy
maurolacy previously approved these changes Oct 22, 2021
Copy link
Contributor

@maurolacy maurolacy 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. Will check the effect this feature has on contract size and report back.

@maurolacy
Copy link
Contributor

Just tested it and this flag has no effect on contract size; the artifact is exactly the same as before.

I would say, let's remove this feature before merging.

@ethanfrey
Copy link
Member

If you turn on the library flag, there are also a lot of pub fn but no entry points beyond those standard ones for allocate, etc build into cosmwasm-std.

This weighs in like 10kb.

So, yeah, pretty much anything not reachable from an entry point should be stripped off when running through the optimizer (maybe wasm-opt not rust, we use both)

Happy to remove that feature flag and keep the rest

@hashedone hashedone dismissed stale reviews from maurolacy and ethanfrey via f44b912 October 25, 2021 08:57
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.

Let's merge

@hashedone hashedone merged commit 2240b6c into main Oct 25, 2021
@hashedone hashedone deleted the 494-cw1-whitelist-ng-followup branch October 25, 2021 10:38
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.

None yet

3 participants