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

Temporarily add Determinism enum from pallet-contracts #1547

Merged
merged 6 commits into from
Dec 19, 2022

Conversation

SkymanOne
Copy link
Contributor

Recent release of substrate-contracts-node introduce new parameter determinism in upload_code() which needs to be specified on the ink! and cargo-contract sides, otherwise no E2E tests can be run with the latest substrate-contracts-node version.
pallet-contracts currently do not expose pallet_contracts::Determinism publicly.
Therefore, the temporary solution is to copy-paste the enum into the ink! codebase until the next release of polkadot branch in substrate and synchronisation of substrate-contracts-node

@SkymanOne SkymanOne changed the title Temporary add Determinism enum from pallet-contracts Temporarily add Determinism enum from pallet-contracts Dec 16, 2022
@HCastano HCastano requested a review from a team as a code owner December 16, 2022 23:30
@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2022

Codecov Report

Merging #1547 (18b8b02) into master (34f9e8c) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1547      +/-   ##
==========================================
- Coverage   71.57%   71.55%   -0.03%     
==========================================
  Files         205      205              
  Lines        6294     6296       +2     
==========================================
  Hits         4505     4505              
- Misses       1789     1791       +2     
Impacted Files Coverage Δ
crates/e2e/src/xts.rs 0.00% <ø> (ø)
crates/allocator/src/bump.rs 86.77% <0.00%> (-1.46%) ⬇️

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

@@ -16,7 +16,7 @@ pub mod flipper {

/// Creates a new flipper smart contract initialized to `false`.
#[ink(constructor)]
pub fn default() -> Self {
pub fn new_default() -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are the new_default changes necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New clippy lints do not allow methods with default() name as they can be confused with the Default::default() implementation. See the the previous clippy output: https://gitlab.parity.io/parity/mirrors/ink/-/jobs/2171790

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this lint's been around a very long time, why is it only being triggered now?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of this new_default method, it made sense to have a default method for the contract. Maybe we can work around it using a the Default trait impl instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a fan of this new_default method, it made sense to have a default method for the contract. Maybe we can work around it using a the Default trait impl instead?

Can you open a follow up issue for this please?

@SkymanOne SkymanOne merged commit 8c7bde3 into master Dec 19, 2022
@SkymanOne SkymanOne deleted the gn/determinism-arg-placeholder branch December 19, 2022 15:29
@@ -28,7 +28,7 @@ variables:
CARGO_TARGET_DIR: "/ci-cache/${CI_PROJECT_NAME}/targets/${CI_COMMIT_REF_NAME}/${CI_JOB_NAME}"
# CI_IMAGE is changed to "-:staging" when the CI image gets rebuilt
# read more https://github.com/paritytech/scripts/pull/244
CI_IMAGE: "paritytech/ink-ci-linux:production"
CI_IMAGE: "paritytech/ink-ci-linux:staging"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should've been rolled back before merging

#[derive(
Debug, Clone, Copy, scale::Encode, scale::Decode, PartialEq, Eq, serde::Serialize,
)]
pub enum Determinism {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing docs

@ascjones ascjones mentioned this pull request Jan 12, 2023
HCastano added a commit that referenced this pull request Jan 23, 2023
* copy Determinism enum from pallet and add it tx calls

* Use `staging` image

* apply clippy suggestions

* rename default constructors to new_default

* fix UI test error message

Co-authored-by: Hernando Castano <hernando@hcastano.com>
@HCastano HCastano mentioned this pull request Jan 24, 2023
@ascjones ascjones mentioned this pull request Feb 15, 2023
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