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

Create the Mother of All Contracts #1245

Merged
merged 26 commits into from May 18, 2022
Merged

Create the Mother of All Contracts #1245

merged 26 commits into from May 18, 2022

Conversation

agryaznov
Copy link
Contributor

@agryaznov agryaznov commented May 3, 2022

This PR should solve #1181.

I decided to give it a narrower name, as Mother of All Contracts implies something really exhaustive, while this contract just solves its particular goal: to be a helpful example for UI developers demonstrating all possible inputs/outputs of an ink! contract.

@athei athei linked an issue May 8, 2022 that may be closed by this pull request
Copy link
Contributor

@athei athei left a comment

Choose a reason for hiding this comment

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

I still thing we should call it mother of all contracts. It should use all ink! features so that UI devs can make sure that they support them properly. Of course some features are not applicable as they are not observable for UIs. But everything that somehow interacts with them should be included in this contract.

examples/bioc/Cargo.toml Outdated Show resolved Hide resolved
examples/bioc/lib.rs Outdated Show resolved Hide resolved
examples/bioc/lib.rs Outdated Show resolved Hide resolved
examples/bioc/lib.rs Outdated Show resolved Hide resolved
examples/bioc/lib.rs Outdated Show resolved Hide resolved
examples/bioc/lib.rs Outdated Show resolved Hide resolved
examples/bioc/lib.rs Outdated Show resolved Hide resolved
@athei athei requested a review from statictype May 10, 2022 09:51
@agryaznov agryaznov changed the title Add Basic Input Output Contract example Create the Mother of All Contracts May 10, 2022
@statictype
Copy link
Contributor

statictype commented May 11, 2022

Could not build the contract to test it out but i can just enumerate the types of inputs we needed to handle in the UI so far.
As for the outputs, it would be sufficient if the contract messages just return the input values.

Inputs/Outputs

  1. Number: u8, i8, i32, u32, i64, u64 - these inputs validate differently in the UI so it would be useful to test them separately. could be done in unit testing if you think it's too much for the Mother
  2. Boolean
  3. Balance
  4. AccountID
  5. Vector
    • Nested Vec - Vec<Vec<Vec<AccountID>>>
    • No generic param Vec - not clear what the default input should be here? found this kind of input but don't remember where
    • Vec<u8> special case, polkadot.js doesn't display a normal Vector input with + and - buttons. should accept either a hex string or a file upload. - see SBP contract ProjectName
  6. Struct - this input basically generates an inner form - would be useful if the struct had all other types as fields
  7. Enum - a nested enum
  8. Option
  9. Hash

Messages

  • should have getters with and without params
  • should have mutating messages with and without params
  • should have multiple constructors with and without params

Events

  • the contract should contain custom events (user defined)

Errors (not sure how to test these, maybe just create some messages that will always fail?)

  • dispatchable failed
  • contract reverted
  • debug messages

@HCastano HCastano added the A-examples [examples] Work item label May 11, 2022
examples/mother/Cargo.toml Outdated Show resolved Hide resolved
examples/mother/Cargo.toml Outdated Show resolved Hide resolved
examples/mother/lib.rs Show resolved Hide resolved
examples/mother/lib.rs Outdated Show resolved Hide resolved
@agryaznov
Copy link
Contributor Author

@statictype thanks for your review!
The contract builds now, please feel free to play with it.

Regarding the input types kindly provided in your list. It seems that most of them are already there, with only these ones left to be added to the contract:

  • Nested Vec - Vec<Vec<Vec>>

Currently we actually have Vec<Option<(AccountId, Balance)>>. Please, let me know if we need nested vectors still?

  • Vec<u8>
  • nested Enum

We have Enum nested into Struct. Would it be enough?

  • Events

Regarding Number types, I would left only BlockNumber (u32) for the contract if you really can cover others by unit tests.

@agryaznov agryaznov requested a review from athei May 12, 2022 18:18
Co-authored-by: Alexander Theißen <alex.theissen@me.com>
@paritytech-cicd-pr
Copy link

paritytech-cicd-pr commented May 12, 2022

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

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

Δ Optimized Size Δ Used Gas Total Optimized Size Total Used Gas
accumulator 1.04 K
adder 2.13 K
contract-introspection 2.37 K
contract-terminate 0.94 K 275_000
contract-transfer 8.13 K 75_000
data-structures 1.73 K
delegate-calls 2.96 K 76_276
delegator 6.38 K 232_281
dns 8.89 K 225_000
erc1155 17.30 K 450_000
erc20 8.49 K 225_000
erc721 11.82 K 600_000
flipper 1.31 K 75_000
forward-calls 2.88 K 151_427
incrementer 1.21 K
mother +12.41 K 12.41 K
multisig 25.12 K 470_156
rand-extension 3.92 K 75_000
seal-code-hash 1.58 K
set-code-hash 1.57 K 150_000
subber 2.15 K
trait-erc20 8.77 K 225_000
trait-flipper 1.00 K 75_000
trait-incrementer 1.19 K 150_000
updated-incrementer 9.62 K
upgradeable-flipper 1.55 K

Link to the run | Last update: Wed May 18 15:36:20 CEST 2022

examples/mother/Cargo.toml Outdated Show resolved Hide resolved
examples/mother/lib.rs Outdated Show resolved Hide resolved
examples/mother/lib.rs Show resolved Hide resolved
@statictype
Copy link
Contributor

statictype commented May 16, 2022

@agryaznov still couldn't build the Mother, but i can answer your questions

Currently we actually have Vec<Option<(AccountId, Balance)>>. Please, let me know if we need nested vectors still?

With this we'd like to test our recursive React component that should support multiple levels of nesting, so we need at least Vec<Vec<<(AccountId, Balance)>>.

We have Enum nested into Struct. Would it be enough?

What i meant here is nested Enum variants, for example:

pub enum MilestoneIndex {
   M1,
   M2,
   M3,
}
pub enum ProjectStatus {
  Applied,
  PostPoned,
  Evaluation,
  Ongoing(MilestoneIndex),
  Alumni,
}

Regarding Number types, I would left only BlockNumber (u32) for the contract if you really can cover others by unit tests.

👍🏻

@athei
Copy link
Contributor

athei commented May 17, 2022

@agryaznov still couldn't build the Mother, but i can answer your questions

It is only the linting that fails. It isn't smart enough to recognize the delegating constructor to initialize the contract.

@agryaznov agryaznov requested a review from athei May 17, 2022 15:16
@agryaznov
Copy link
Contributor Author

@statictype the contract is updated, looking forward your feedback!

@codecov-commenter
Copy link

codecov-commenter commented May 18, 2022

Codecov Report

Merging #1245 (3491cfd) into master (b6dd935) will decrease coverage by 0.15%.
The diff coverage is n/a.

❗ Current head 3491cfd differs from pull request most recent head ca2963f. Consider uploading reports for the commit ca2963f to get more accurate results

@@            Coverage Diff             @@
##           master    #1245      +/-   ##
==========================================
- Coverage   78.74%   78.58%   -0.16%     
==========================================
  Files         228      228              
  Lines        8675     8654      -21     
==========================================
- Hits         6831     6801      -30     
- Misses       1844     1853       +9     
Impacted Files Coverage Δ
crates/primitives/src/key_ptr.rs 75.00% <0.00%> (-25.00%) ⬇️
crates/engine/src/chain_extension.rs 19.04% <0.00%> (-23.81%) ⬇️
crates/engine/src/ext.rs 58.53% <0.00%> (-11.64%) ⬇️
crates/engine/src/test_api.rs 85.71% <0.00%> (-5.50%) ⬇️
crates/env/src/api.rs 30.88% <0.00%> (-2.93%) ⬇️
crates/env/src/engine/off_chain/impls.rs 41.97% <0.00%> (-2.74%) ⬇️
crates/lang/ir/src/ir/attrs.rs 81.44% <0.00%> (-0.84%) ⬇️
crates/lang/src/env_access.rs 12.00% <0.00%> (ø)
crates/storage/src/lazy/mapping.rs 54.28% <0.00%> (ø)
crates/allocator/src/bump.rs
... and 1 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 b6dd935...ca2963f. Read the comment docs.

@statictype
Copy link
Contributor

all good from my side, i used it with the UI and already revealed a few bugs. thanks so much!

examples/mother/lib.rs Outdated Show resolved Hide resolved
@agryaznov
Copy link
Contributor Author

agryaznov commented May 18, 2022

bot merge

@agryaznov agryaznov merged commit 8ffb7c0 into master May 18, 2022
@agryaznov agryaznov deleted the ag-bioc branch May 18, 2022 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-examples [examples] Work item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create the mother of all contracts
7 participants