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

Update CONTRIBUTING.md: add Backwards Compatibility guide and update questions channels #1304

Merged
merged 7 commits into from
Jul 14, 2022

Conversation

agryaznov
Copy link
Contributor

follow up to #1284

@paritytech-cicd-pr
Copy link

paritytech-cicd-pr commented Jun 27, 2022

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

These are the results when building the examples/* contracts from this branch with cargo-contract 1.4.0-25bfb96 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-introspection 2.32 K
contract-terminate 0.92 K 275_000
contract-transfer 8.31 K 75_000
data-structures 1.73 K
delegate-calls 2.89 K 76_242
delegator -1 6.34 K 232_135
dns 8.81 K 225_000
erc1155 16.74 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_411
incrementer 1.14 K
mother 12.16 K
multisig 25.28 K 470_240
rand-extension 3.79 K 75_000
seal-code-hash 1.39 K
seal-ecdsa 1.71 K
set-code-hash 1.49 K 150_000
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.72 K
upgradeable-flipper 1.48 K

Link to the run | Last update: Tue Jun 28 12:16:47 CEST 2022

@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2022

Codecov Report

Merging #1304 (1c57c9b) into master (cf67e08) will decrease coverage by 24.83%.
The diff coverage is n/a.

❗ Current head 1c57c9b differs from pull request most recent head a32251c. Consider uploading reports for the commit a32251c to get more accurate results

@@             Coverage Diff             @@
##           master    #1304       +/-   ##
===========================================
- Coverage   71.94%   47.11%   -24.84%     
===========================================
  Files         177      176        -1     
  Lines        5950     5920       -30     
===========================================
- Hits         4281     2789     -1492     
- Misses       1669     3131     +1462     
Impacted Files Coverage Δ
crates/lang/codegen/src/traits.rs 0.00% <0.00%> (-100.00%) ⬇️
crates/lang/ir/src/ir/contract.rs 0.00% <0.00%> (-100.00%) ⬇️
crates/lang/codegen/src/generator/env.rs 0.00% <0.00%> (-100.00%) ⬇️
crates/lang/codegen/src/generator/mod.rs 0.00% <0.00%> (-100.00%) ⬇️
crates/lang/codegen/src/enforced_error.rs 0.00% <0.00%> (-100.00%) ⬇️
crates/lang/codegen/src/generator/blake2b.rs 0.00% <0.00%> (-100.00%) ⬇️
crates/lang/codegen/src/generator/storage.rs 0.00% <0.00%> (-100.00%) ⬇️
crates/lang/codegen/src/generator/arg_list.rs 0.00% <0.00%> (-100.00%) ⬇️
crates/lang/codegen/src/generator/contract.rs 0.00% <0.00%> (-100.00%) ⬇️
crates/lang/codegen/src/generator/selector.rs 0.00% <0.00%> (-100.00%) ⬇️
... and 48 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 cf67e08...a32251c. Read the comment docs.

@agryaznov agryaznov changed the title Add Backwards Compatibility guide to CONTRIBUTING.md Update CONTRIBUTING.md: add Backwards Compatibility guide and update questions channels Jun 27, 2022
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated
@@ -64,6 +65,23 @@ Following these will ensure that your pull request is going to be accepted.
This might seem pedantic but we believe that in essence this is going to improve overall comment and documentation quality.
1. If possible try to sign your commits, e.g. using GPG keys. For more information about this go [here](https://help.github.com/en/articles/signing-commits).

### Backwards Compatibility

ink! and pallet_contracts are the projects under active development. As contracts API functions evolve to their newer versions, we need to introduce them in ink! in a way that still keeps current *MAJOR* ink! version compatible with elder substrate nodes built with the pallet_contracts version having not this new functionality.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ink! and pallet_contracts are the projects under active development. As contracts API functions evolve to their newer versions, we need to introduce them in ink! in a way that still keeps current *MAJOR* ink! version compatible with elder substrate nodes built with the pallet_contracts version having not this new functionality.
ink! and pallet_contracts are the projects under active development. As contracts API functions (denoted by the `seal_*` prefix) evolve to their newer versions, we need to introduce them in ink! in a way that still keeps current *MAJOR* ink! version compatible with older Substrate nodes. These older nodes may have a version of the Contracts pallet which does not support new seal APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

left it to be Andrew's variant for the sake of concise

CONTRIBUTING.md Outdated

In order to achieve this, please stick to the following workflow.

Imagine there is a `[seal0] function()` in pallet\_contracts API and our `ink_env::function()` uses its import under the hood.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Imagine there is a `[seal0] function()` in pallet\_contracts API and our `ink_env::function()` uses its import under the hood.
Imagine there is a `[seal0] function()` in the Contract pallet API and our `ink_env::function()` imports it under the hood.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well function does not import, it uses an imported one

CONTRIBUTING.md Outdated

1. Mark the old `ink_env::function()` (which depends on the imported `[seal0]` function) with the `#[deprecated]` attribute.
Please, specify the `since` field with the ink! version since which this function is deprecated in favor of the newest one, and is left there only for the backwards compatibility purposes. Specifing some additional helpful info in the `note` field could also be a good idea.
2. Name the new function (which depends on the `[seal1] function()`) somehow descriptive like `ink_env::function_whats_special()`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Mark all of these as 1, the markdown render will handle the numbering

Copy link
Contributor Author

@agryaznov agryaznov Jun 28, 2022

Choose a reason for hiding this comment

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

well, this is an optional feature, btw it will work so even if you mark it with a randomly assigned set of integer numbers as well. However, I think it's good to have it numbered properly in the source itself.

CONTRIBUTING.md Outdated
1. Mark the old `ink_env::function()` (which depends on the imported `[seal0]` function) with the `#[deprecated]` attribute.
Please, specify the `since` field with the ink! version since which this function is deprecated in favor of the newest one, and is left there only for the backwards compatibility purposes. Specifing some additional helpful info in the `note` field could also be a good idea.
2. Name the new function (which depends on the `[seal1] function()`) somehow descriptive like `ink_env::function_whats_special()`.
3. Always add the `# Compatibility` section to the docs comment of the new function.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
3. Always add the `# Compatibility` section to the docs comment of the new function.
3. Add a `# Compatibility` section to the docs comment of the new function.

CONTRIBUTING.md Outdated Show resolved Hide resolved
@HCastano
Copy link
Contributor

Oh btw, can you wrap everything to 90 characters?

agryaznov and others added 2 commits June 28, 2022 12:33
Co-authored-by: Andrew Jones <ascjones@gmail.com>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
@HCastano HCastano merged commit f0beec3 into master Jul 14, 2022
@HCastano HCastano deleted the ag-contributing branch July 14, 2022 18:07
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.

5 participants