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 trait-incrementer example smart contract #932

Merged
merged 2 commits into from
Sep 21, 2021

Conversation

Robbepop
Copy link
Collaborator

New example ink! smart contract extracted from #665.

@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2021

Codecov Report

Merging #932 (8872977) into master (41b0a1c) will increase coverage by 2.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #932      +/-   ##
==========================================
+ Coverage   84.20%   86.21%   +2.00%     
==========================================
  Files         173      156      -17     
  Lines        7939     6353    -1586     
==========================================
- Hits         6685     5477    -1208     
+ Misses       1254      876     -378     
Impacted Files Coverage Δ
crates/env/src/engine/off_chain/db/chain_spec.rs 79.54% <0.00%> (-20.46%) ⬇️
crates/lang/ir/src/ir/item_impl/mod.rs 73.58% <0.00%> (-17.80%) ⬇️
crates/env/src/engine/off_chain/chain_extension.rs 57.14% <0.00%> (-7.86%) ⬇️
crates/lang/codegen/src/generator/metadata.rs 90.90% <0.00%> (-6.66%) ⬇️
crates/env/src/engine/off_chain/db/events.rs 93.54% <0.00%> (-6.46%) ⬇️
crates/lang/ir/src/ir/item_impl/impl_item.rs 51.35% <0.00%> (-6.10%) ⬇️
crates/env/src/engine/off_chain/db/accounts.rs 81.15% <0.00%> (-5.43%) ⬇️
crates/lang/ir/src/ir/idents_lint.rs 68.42% <0.00%> (-3.01%) ⬇️
crates/env/src/engine/off_chain/mod.rs 86.95% <0.00%> (-2.66%) ⬇️
crates/lang/ir/src/ir/config.rs 88.88% <0.00%> (-2.61%) ⬇️
... and 60 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 41b0a1c...8872977. Read the comment docs.

Copy link
Contributor

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

Small comments from my end, but looks good 👍

examples/trait-incrementer/Cargo.toml Outdated Show resolved Hide resolved
examples/trait-incrementer/lib.rs Outdated Show resolved Hide resolved
examples/trait-incrementer/lib.rs Outdated Show resolved Hide resolved
examples/trait-incrementer/lib.rs Outdated Show resolved Hide resolved
examples/trait-incrementer/lib.rs Show resolved Hide resolved
/// Allows to increment and get the current value.
#[ink::trait_definition]
pub trait Increment {
/// Increments the current value of the implementer by one (1).
Copy link
Contributor

Choose a reason for hiding this comment

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

My nitpick here was that it's up to the trait implementer to choose the amount inc increases by 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do not agree because this would violate the invariant stated by the docs of the trait's message.

Copy link
Collaborator Author

@Robbepop Robbepop Sep 21, 2021

Choose a reason for hiding this comment

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

Just because our concrete incrementer does _also_support inc_by does not mean this invariant must not be uphold for the trait implementation in particular. You could say the demonstrated concrete incrementer smart contract provides a richer API than what the ink! trait mandates but that's always fine, also in typical Rust contexts.

@Robbepop Robbepop merged commit 7577c67 into master Sep 21, 2021
@Robbepop Robbepop deleted the robin-add-trait-incrementer branch September 21, 2021 16:09
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.

3 participants