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

Release 3.3.0 with backwards compatibility fixes #1284

Merged
merged 27 commits into from
Jun 22, 2022
Merged

Conversation

agryaznov
Copy link
Contributor

@agryaznov agryaznov commented Jun 10, 2022

Context: user-reported issues on our SE (first and second) unveiled backward incompatibility introduced in the 3.1.0 release.

The following has been done to restore backward compatibility:

  • Reverted backward-incompatible piece of #1233: removal of eth_compatibility crate
  • Reverted backward-incompatible piece of #1224: dependency on [seal1] seal_set_storage
  • Reverted "Optimise deny_payment. Use eerywhere semantic of deny. (#1267)"
    (this one is to restore compatibiliy between minor versions of ink! crates; see @HCastano SE answer in this regard)

All these reverted breaking changes are subject to be included in the upcoming MAJOR ink! release 4.
Meanwhile, to continue support and delivering fixes for ink! 3, branch v3.x.x was created from 3.2.0 tag.


This closes #1278.

@HCastano
Copy link
Contributor

Sorry, will take a look at this tomorrow. We should get this out soon

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.

Main question I have has to do with the use of the *_compat methods

crates/env/src/backend.rs Outdated Show resolved Hide resolved
crates/env/src/api.rs Outdated Show resolved Hide resolved
crates/engine/src/ext.rs Outdated Show resolved Hide resolved
crates/storage/src/traits/mod.rs Outdated Show resolved Hide resolved
RELEASES.md Outdated Show resolved Hide resolved
RELEASES.md Outdated Show resolved Hide resolved
RELEASES.md Outdated Show resolved Hide resolved
RELEASES.md Outdated Show resolved Hide resolved
RELEASES.md Outdated Show resolved Hide resolved
RELEASES.md Outdated Show resolved Hide resolved
RELEASES.md Outdated Show resolved Hide resolved
agryaznov and others added 6 commits June 15, 2022 17:32
Co-authored-by: Michael Müller <mich@elmueller.net>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
@agryaznov agryaznov changed the title Release 3.3.0 with backward compatibility fixes Release 3.3.0 with backwards compatibility fixes Jun 15, 2022
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.

Just realized that this won't work.

We can't leave set_contract_storage as is, we need to revert it to not return anything, otherwise code like:

#[ink(message)]
pub fn foo(&self) {
    ink_env::set_contract_storage(&Default::default(), &1)
}

won't compile (even though it did in 3.0.1).

Instead of having the compat functions we need with_size functions (or something similarly named).

@agryaznov
Copy link
Contributor Author

agryaznov commented Jun 16, 2022

We can't leave set_contract_storage as is, we need to revert it to not return anything, otherwise code like:

Nice catch! Fixed for ink_env::set_contract_storage().

Instead of having the compat functions we need with_size functions (or something similarly named).

I renamed the user-facing api function. This should solve the issue I think. As a general rule, having _compat named old functions on the backend side should make it simple to remove them all as a general cleanup before a next MAJOR release.

@HCastano
Copy link
Contributor

Can you remove all the compat functions and only use the with_size ones? It'll make things more consistent throughout, ensure we don't accidentally break SemVer somewhere we weren't expecting, and make removing the old functions easier when we move to 4.0

@agryaznov
Copy link
Contributor Author

Updated according to the discussed naming policy + added #[deprecated] attribute

@agryaznov agryaznov requested a review from HCastano June 16, 2022 15:55
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.

Some small things, but we're almost there 💪

crates/env/src/api.rs Outdated Show resolved Hide resolved
crates/env/src/api.rs Outdated Show resolved Hide resolved
crates/env/src/backend.rs Outdated Show resolved Hide resolved
crates/storage/src/traits/impls/mod.rs Outdated Show resolved Hide resolved
crates/storage/src/traits/mod.rs Outdated Show resolved Hide resolved
crates/env/src/engine/on_chain/ext.rs Show resolved Hide resolved
crates/env/src/api.rs Outdated Show resolved Hide resolved
crates/env/src/api.rs Outdated Show resolved Hide resolved
crates/storage/src/traits/mod.rs Outdated Show resolved Hide resolved
crates/env/src/api.rs Outdated Show resolved Hide resolved
agryaznov and others added 2 commits June 17, 2022 13:04
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
@agryaznov agryaznov requested a review from HCastano June 17, 2022 10:47
crates/storage/src/lazy/mapping.rs Outdated Show resolved Hide resolved
crates/storage/src/traits/mod.rs Show resolved Hide resolved
crates/storage/src/traits/mod.rs Outdated Show resolved Hide resolved
crates/storage/src/traits/mod.rs Outdated Show resolved Hide resolved
crates/env/src/engine/on_chain/ext.rs Show resolved Hide resolved
crates/env/src/api.rs Outdated Show resolved Hide resolved
crates/engine/src/ext.rs Outdated Show resolved Hide resolved
@agryaznov
Copy link
Contributor Author

agryaznov commented Jun 20, 2022

also fixed spellcheck config (was done wrong in #1294) to make CI happy

@paritytech-cicd-pr
Copy link

paritytech-cicd-pr commented Jun 20, 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.4.0-25bfb96 and comparing them to ink! master:

Δ Optimized Size Δ Used Gas Total Optimized Size Total Used Gas
accumulator +0.04 K 1.03 K
adder +0.10 K 2.13 K
contract-introspection +0.05 K 2.37 K
contract-terminate +0.02 K 0.94 K 275_000
contract-transfer 8.31 K 75_000
data-structures +0.01 K 1.73 K
delegate-calls +0.07 K +22 2.95 K 76_264
delegator +0.03 K +98 6.37 K 232_234
dns +0.08 K 8.89 K 225_000
erc1155 +0.55 K 17.29 K 450_000
erc20 +0.07 K 8.49 K 225_000
erc721 +0.19 K 11.81 K 600_000
flipper +0.07 K 1.31 K 75_000
forward-calls +0.01 K +5 2.88 K 151_416
incrementer +0.07 K 1.21 K
mother +0.20 K 12.36 K
multisig +0.01 K +6 25.29 K 470_246
rand-extension +0.13 K 3.92 K 75_000
seal-code-hash +0.19 K 1.58 K
seal-ecdsa +0.09 K 1.80 K
set-code-hash +0.07 K 1.57 K 150_000
subber +0.09 K 2.15 K
trait-erc20 +0.07 K 8.76 K 225_000
trait-flipper +0.03 K 1.00 K 75_000
trait-incrementer +0.06 K 1.19 K 150_000
updated-incrementer +0.08 K 9.80 K
upgradeable-flipper +0.07 K 1.55 K

Link to the run | Last update: Tue Jun 21 21:56:23 CEST 2022

@codecov-commenter
Copy link

codecov-commenter commented Jun 20, 2022

Codecov Report

❗ No coverage uploaded for pull request base (v3.x.x@f403c6e). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             v3.x.x    #1284   +/-   ##
=========================================
  Coverage          ?   71.87%           
=========================================
  Files             ?      178           
  Lines             ?     5984           
  Branches          ?        0           
=========================================
  Hits              ?     4301           
  Misses            ?     1683           
  Partials          ?        0           

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 f403c6e...28ee702. Read the comment docs.

@agryaznov agryaznov requested a review from HCastano June 21, 2022 14:34
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.

Looks good! @ascjones can you take a look before we merge it?

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

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

6 participants