openethereum / sol-rs

Solaris - Solidity testing framework in Rust.
GNU General Public License v3.0
54 stars 14 forks source link

adapt to upstream changes, make tests and CI pass again #31

Closed snd closed 6 years ago

snd commented 6 years ago

resolve #30 among other changes

snd commented 6 years ago

blocked. will continue this once the following 2 PRs are merged:

https://github.com/paritytech/ethabi/pull/85

https://github.com/paritytech/parity/pull/8258

tomusdrw commented 6 years ago

@snd any chance we could get this updated if you have some spare time?

snd commented 6 years ago

@tomusdrw yes! i'll try to look at it later today

snd commented 6 years ago

@tomusdrw are you fine with changing the no longer working occurrences like contract.function(input1, input2).transact(&evm) to evm.transact(contract.function(input1, input2)?

this is easier to implement. it also makes more sense in my opinion to pass the encoded inputs to the evm instead of passing the evm to the encoded inputs.

tomusdrw commented 6 years ago

@snd Fine with me, let's see how the API looks like, we can always add an extension trait and blanket impl that trait for every T: ContractFunction.

My gut feeling is that .call() is a tiny bit better cause it's easier to chain it (with .and_then/wait/, etc). But I don't see any reasons why we couldn't support both ways initially to see which one is more ergonomic long-term.

snd commented 6 years ago

@tomusdrw

we can always add an extension trait and blanket impl that trait for every T: ContractFunction

that would simply call evm.call/transact - very easy to add.

But I don't see any reasons why we couldn't support both ways initially to see which one is more ergonomic long-term.

👍

snd commented 6 years ago

@tomusdrw there's some "shitty" detection of out of gas: https://github.com/paritytech/sol-rs/blob/041d44875d5e38971bc082be16d1e0acd43a3aa1/solaris/src/evm.rs#L163

there's already an OutOfGas variant on evm::vm::Error https://github.com/paritytech/parity/blob/25b35ebddd11e400d48c891bf5ea4b75851ea4cf/ethcore/vm/src/error.rs#L30

can we remove the "shitty" detection since it seems to be covered by the evm error? not sure whether it's actually detected within evm.

taking the time to add proper error handling with error-chain.

snd commented 6 years ago

running into a weird issue where ethabi-derive generates () as output type for the functions in the sol-rs test contracts. will pin down the issue next week

snd commented 6 years ago

travis fails at: https://travis-ci.org/paritytech/sol-rs/jobs/386427116#L994

error[E0308]: mismatched types
  --> tests/tests/test.rs:35:27
   |
35 |       let output: Address = evm.with_sender(sender)
   |  ___________________________^
36 | |         .call(contract.functions().get_sender())
37 | |         .unwrap();
   | |_________________^ expected struct `types::H160`, found ()
   |
   = note: expected type `types::H160`
              found type `()`

for some reason ethabi-derive generates () as ContractFunction::Output type for the get_sender function. Output should be H160 (Address)

here's the function in solidity: https://github.com/paritytech/sol-rs/blob/2053fe1d805dc2eb5d43abf8de81abe1a401fcb4/tests/contracts/test.sol#L2

i've confirmed it by generating docs for the derived contract.

@tomusdrw @axelchalon any ideas?

snd commented 6 years ago

CI finally passes - yay!

i'll make another pass over this soon.

feel free to start reviewing though

snd commented 6 years ago

ready for review. let's get this merged :)

snd commented 6 years ago

@tomusdrw would be fantastic to get a review sometime this week : )

snd commented 6 years ago

@tomusdrw grumbles addressed.

"squash and merge" seems to be the default merge option for this repo. let me know what you prefer