Skip to content
This repository has been archived by the owner on Feb 14, 2021. It is now read-only.

Add tests for create2 in creator #51

Merged
merged 9 commits into from
Aug 6, 2018
Merged

Add tests for create2 in creator #51

merged 9 commits into from
Aug 6, 2018

Conversation

sorpaas
Copy link
Contributor

@sorpaas sorpaas commented Aug 6, 2018

rel openethereum/parity-ethereum#9277

  • Fixes all scripts to use /usr/bin/env, otherwise it breaks on non-FHS Linux.
  • Fixes build-all.sh to build for newest version of wasm-build.
  • Allow custom features (like kip4) to be enabled for specific tests, in gen.
  • Add tests for create2 in creator.rs.

@sorpaas sorpaas requested review from pepyakin and NikVolf August 6, 2018 09:35
Copy link
Contributor

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

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

Nice!

src/alloc.rs Outdated

#[no_mangle]
pub fn deploy() { }
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why these are needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wasm-build would complain if no deploy symbol is found: https://github.com/paritytech/wasm-utils/blob/master/src/build.rs#L110
In the binary, we always set constructor to true and build the deploy wrapped version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sorpaas

there is a bug in wasm-build 0.3.0 with this constructor=true always

you might try pwasm-utils-cli 0.2.0 (cargo install pwasm-utils-cli --version 0.2.0) or wait until @fckt fix it

Copy link
Contributor

Choose a reason for hiding this comment

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

@sorpaas
Copy link
Contributor Author

sorpaas commented Aug 6, 2018

Only merge after openethereum/pwasm-ethereum#12 -- we would need to change the pwasm-ethereum branch from kip4 to master.

gen/main.rs Outdated
pwasm-std = "0.9.0"
pwasm-ethereum = "0.5.0"
bigint = { version = "4", default-features = false }
pwasm-std = {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

could just be updated to 0.10 for all?

gen/main.rs Outdated
pwasm-ethereum = "0.5.0"
bigint = { version = "4", default-features = false }
pwasm-std = {}
pwasm-ethereum = {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

could be just 0.6 ?

src/creator.rs Outdated
} else {
logger::debug("Error creating contract");
}

if let Ok(addr) = create2(value() / U256::from(2), H256::default(), &input()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe let's use some non-zero salt to actually test the argument reaches the runtime after?


#[no_mangle]
pub fn deploy() { }
Copy link
Collaborator

@NikVolf NikVolf Aug 6, 2018

Choose a reason for hiding this comment

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

there should not be deploy in any of those tests

with deploy, they will get packed as constructors, while they need to get packed as raw wasm module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NikVolf With no deploy wasm-build would refuse to pack those modules: https://github.com/paritytech/wasm-utils/blob/master/src/build.rs#L110

Copy link
Collaborator

Choose a reason for hiding this comment

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

should be fixed now

Copy link
Collaborator

Choose a reason for hiding this comment

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

(reinstall pwasm-utils-cli)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed all deploy symbols!

@NikVolf
Copy link
Collaborator

NikVolf commented Aug 6, 2018

You might want to avoid including all recompiled wasm binaries in the diff and include only creator.wasm

Otherwise you will have to update gas costs for all tests in parity-ethereum repo

but it's up to you!

@lexfrl
Copy link
Contributor

lexfrl commented Aug 6, 2018

Otherwise you will have to update gas costs for all tests in parity-ethereum repo

And in case you'll decide to do so, please update the rust-toolchain file to use nightly-2018-07-24 to sync in with other wasm repos, like https://github.com/paritytech/pwasm-std/blob/master/rust-toolchain

@sorpaas
Copy link
Contributor Author

sorpaas commented Aug 6, 2018

Some of the gas costs are already changed due to update to pwasm-ethereum. So I think I would just fix the gas costs. And updated rust-toolchain!

@NikVolf NikVolf merged commit 242b8d8 into master Aug 6, 2018
@NikVolf NikVolf deleted the kip4-tests branch August 6, 2018 14:25
@sorpaas sorpaas restored the kip4-tests branch August 6, 2018 16:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants