Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Move linting into ink repo #1361

Merged
merged 6 commits into from
Aug 24, 2022
Merged

Move linting into ink repo #1361

merged 6 commits into from
Aug 24, 2022

Conversation

athei
Copy link
Contributor

@athei athei commented Aug 23, 2022

In preparation for the changes to cargo contract which will make the lints being build as part of the contract build we move it here. The lints are tightly coupled to the ink! code. There is no reason why they should exist in the cargo contract repo.

@athei athei requested review from a team, cmichi, ascjones and HCastano as code owners August 23, 2022 11:55
@paritytech-cicd-pr
Copy link

paritytech-cicd-pr commented Aug 23, 2022

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

⚠️ The ink! master is ahead of your branch, this might skew the comparison data below.

These are the results when building the examples/* contracts from this branch with cargo-contract 1.5.0-ef06f4d and comparing them to ink! master:

Δ Optimized Size Δ Used Gas Total Optimized Size Total Used Gas
accumulator 1.00 K
adder 2.04 K
contract-terminate 0.92 K 275_000
contract-transfer 8.36 K 75_000
delegate-calls 2.89 K 76_242
delegator 6.34 K 232_138
dns 8.81 K 225_000
erc1155 17.15 K 450_000
erc20 8.42 K 225_000
erc721 11.62 K 600_000
flipper 1.24 K 75_000
forward-calls 2.87 K 151_412
incrementer 1.14 K
mother 12.24 K
multisig 25.79 K 470_483
payment-channel 7.95 K
rand-extension 3.79 K 75_000
set-code-hash 1.49 K
subber 2.06 K
trait-erc20 8.69 K 225_000
trait-flipper 0.97 K 75_000
trait-incrementer 1.12 K 150_000
updated-incrementer 9.78 K
upgradeable-flipper 1.48 K

Link to the run | Last update: Tue Aug 23 21:25:12 CEST 2022

Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM ❤️

athei and others added 2 commits August 23, 2022 19:12
Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>
Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>
@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2022

Codecov Report

Merging #1361 (8f3f537) into master (0941e38) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1361   +/-   ##
=======================================
  Coverage   71.96%   71.96%           
=======================================
  Files         177      177           
  Lines        5928     5928           
=======================================
  Hits         4266     4266           
  Misses       1662     1662           

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

[package.metadata.rust-analyzer]
rustc_private = true

[workspace]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this?

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. The driver does not build without that. All the examples in the dylint repo have this, too.

This crate uses [`dylint`](https://github.com/trailofbits/dylint) to define custom
linting rules for [ink!](https://github.com/paritytech/ink);

It is not part of the workspace because it needs a custom linker to be built.
Copy link
Contributor

Choose a reason for hiding this comment

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

I was about to ask if we could include this in the ink! workspace, haha

athei and others added 2 commits August 23, 2022 20:50
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
@athei athei merged commit 147e04b into master Aug 24, 2022
@athei athei deleted the at/linting branch August 24, 2022 06:59
cmichi pushed a commit that referenced this pull request Aug 31, 2022
* Move linting into ink repo

* Fix CI

* Apply suggestions from code review

Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>

* Update .gitlab-ci.yml

Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>

* Update linting/Cargo.toml

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

* Update linting/Cargo.toml

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
xermicus pushed a commit that referenced this pull request Sep 8, 2022
* Move linting into ink repo

* Fix CI

* Apply suggestions from code review

Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>

* Update .gitlab-ci.yml

Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>

* Update linting/Cargo.toml

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

* Update linting/Cargo.toml

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants