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

idea for better rust wasm contract development/testing experience #10

Open
snd opened this issue Nov 13, 2017 · 29 comments
Open

idea for better rust wasm contract development/testing experience #10

snd opened this issue Nov 13, 2017 · 29 comments

Comments

@snd
Copy link
Contributor

snd commented Nov 13, 2017

not sure if this is the right repo for this issue. if someone points me to a better repo i'll move the issue there.

i know rust wasm contract development is super early days. even so the contract developer/tester experience could already be improved a lot.

room for improvement imho:

it would be great if we could write rust contracts a bit like this (wishful thinking):

#[contract]
mod token_contract {
    static TOTAL_SUPPLY_KEY: H256 = H256([2,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]);
    static OWNER_KEY: H256 = H256([3,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]);

    fn constructor<C: Context>(&mut context: C, total_supply: U256) {
        let sender = context.sender();
        // Set up the total supply for the token
        context.storage().write(&TOTAL_SUPPLY_KEY, &total_supply.into()).unwrap();
        // Give all tokens to the contract owner
        context.storage().write(&balance_key(&sender), &total_supply.into()).unwrap();
        // Set the contract owner
        context.storage().write(&OWNER_KEY, &H256::from(sender).into()).unwrap();
    }

    fn totalSupply<C: Context>(&context: C) -> U256 {
        context.storage().read(&TOTAL_SUPPLY_KEY).unwrap_or([0u8; 32]).into()
    }

    // ...
}

this looks much cleaner to me.

it requires no knowledge about global mutable state to understand.

testing would be more natural, require no macros and no setting of global mutable state:

#[test]
fn should_succeed_in_creating_max_possible_amount_of_tokens() {
    let mut context = SomeContextBuilder::new().build();
    // set total supply to maximum value of an unsigned 256 bit integer
    let total_supply = U256::from_dec_str("115792089237316195423570985008687907853269984665640564039457584007913129639935").unwrap();
    assert_eq!(total_supply, U256::max_value());
    constructor(&mut context, total_supply);
    assert_eq!(totalSupply(&context), total_supply);
}

what do you think?

@snd
Copy link
Contributor Author

snd commented Nov 13, 2017

for actually using contracts (not testing) there would be a default Context implementation which would delegate to extern "C" fn

@snd
Copy link
Contributor Author

snd commented Nov 13, 2017

to clarify:

i'm proposing an attribute or macro which makes a smart contract and it's ABI from a module like definition.

functions within that definition take a C: Context as the first argument.
this replaces the pwasm_std::externs and pwasm_std::storage. that makes things more flexible and makes testing straightforward.

less boilerplate because there no longer is a need to specify both trait and impl for a contract.
no weird unused self references in trait impl of the contract.

@lexfrl
Copy link
Contributor

lexfrl commented Nov 13, 2017

I like your ideas. The way we are mocking externs right now seems not very nice, to be honest.
pwasm-std could provide the real context with external calls and we could pass it in _call.

Maybe context.storage().write instead of storage_write (and context.storage().read) is a bit overkill..
@NikVolf and @pepyakin . Guys what do you think?

@snd
Copy link
Contributor Author

snd commented Nov 13, 2017

@fckt i prefer context.storage_write over context.storage().write as well

@snd
Copy link
Contributor Author

snd commented Nov 13, 2017

@fckt

pwasm-std could provide the real context with external calls and we could pass it in _call.

👍

@NikVolf
Copy link
Contributor

NikVolf commented Nov 13, 2017

such approach has a lot of downsides

  • you can't implement multiple contracts with known interface (trait) this way
  • you can't split interface (trait) and it's implementation
  • you can't generate abi if you know only interface (trait)

@snd
Copy link
Contributor Author

snd commented Nov 13, 2017

@NikVolf great that we start a discussion on this

the points you raise focus on the removal of the trait in the example.

do you see any downsides to passing in a C: Context instead of
using pwasm_std::storage and pwasm_std::externs?

i see that as independent from removing the trait.
one could remove the trait without switching to context or
switch to context without removing the trait.

you can't implement multiple contracts with known interface (trait) this way

good point. but i'm sure one could model the problem in a way where one could use traits if that is needed and not use traits if they are not needed and would just be boilerplate.

you can't split interface (trait) and it's implementation

other than 1) what is gained by splitting trait and implementation?

you can't generate abi if you know only interface (trait)

can you elaborate?

seems like you currently generate the abi using only the trait:
https://github.com/paritytech/pwasm-token-example/blob/45b3d2864ae51bd297c2a753340870261b9a4716/src/token.rs#L50

i'm sure one can derive the abi from the trait, a module like thing or
anything else that contains enough information.

@lexfrl
Copy link
Contributor

lexfrl commented Nov 13, 2017

Better to keep contract Endpoint and Client generation using trait, but I like idea of context..

@NikVolf
Copy link
Contributor

NikVolf commented Nov 13, 2017

@snd yeah, downside with Context is pretty simple - you should always prefer implementation complexity over public api complexity. Currently pwasm-std api is pretty simple and consist only of functions, introducing Context there and making contracts interface generic over this Context increases public api complexity, which should (and can) be avoided.

other than 1) what is gained by splitting trait and implementation?

it's easy, i can declare trait in one crate and other crate will just import this trait and implement it for it's own structures, with abi already defined as exactly the same across all implementations.

So user, to guarantee that his contract is compliant to the specific abi, just implements the trait and that's all.

@lexfrl
Copy link
Contributor

lexfrl commented Nov 13, 2017

it's easy, i can declare trait in one crate and other crate will just import this trait and implement it for it's own structures, with abi already defined as exactly the same across all implementations.

So user, to guarantee that his contract is compliant to the specific abi, just implements the trait and that's all.

Like Solidity interfaces
http://solidity.readthedocs.io/en/develop/contracts.html#interfaces

@snd
Copy link
Contributor Author

snd commented Nov 13, 2017

the trait stuff is clear to me. just thought 1) and 2) were essentially the same point and wanted to know whether there's more to it.

@NikVolf
Copy link
Contributor

NikVolf commented Nov 13, 2017

On the second thought, idea with context might be useful, if traits will not be generic over it.
@fckt might come with proof-of-concept in this repo (it can be achieved without touching pwasm-std at all at this point)

@lexfrl
Copy link
Contributor

lexfrl commented Nov 13, 2017

Like so https://play.rust-lang.org/?gist=6570adb28cbd3bb1c139e08389a6a12b&version=stable

@snd
Copy link
Contributor Author

snd commented Nov 13, 2017

Like so https://play.rust-lang.org/?gist=6570adb28cbd3bb1c139e08389a6a12b&version=stable

i already like this much better than the current way 👍

i don't like that the following nontrivial boilerplate is now required for every contract:

struct Token<'a, T: 'a + Context> {
    context: &'a mut T
}

impl<'a, T: 'a + Context> Token<'a, T> {
    fn new (context: &'a mut T) -> Token<'a, T> {
        Token{ context: context }
    }
}

one could write a macro to automate that. but macros obfuscate things and add complexity. not saying one should never use them but a solution without them is often simpler.

the generics with lifetime params are not the easiest and might result in difficult error messages.
would be nice if we could find a solution without that.

let's keep iterating!

@snd
Copy link
Contributor Author

snd commented Nov 13, 2017

why ctor if i may ask? unless one comes from C# it will cause confusion. confusion leads to bugs.
why not constructor? it's unmistakeable!

@NikVolf
Copy link
Contributor

NikVolf commented Nov 13, 2017

Just wanted to note about gist is that better always require &mut self in the Context. Rust &mut is not only about mutability, but also about uniqueness, and Context should always be unique.

Not to mention that tests might want to write some logging data when storage_read is invoked, for example.

@pepyakin
Copy link
Contributor

Can we just have:

  • impl Context for &mut TestContext
  • impl Context for RealContext, where RealContext is just struct RealContext

?

https://play.rust-lang.org/?gist=99209e78f4bbd2de7d8df909f9d4a2e4&version=stable

@lexfrl
Copy link
Contributor

lexfrl commented Nov 13, 2017

@pepyakin yeap, the context gives us this flexibility. In the _call user could use RealContext with externs. In tests he could mock it.

@lexfrl
Copy link
Contributor

lexfrl commented Nov 13, 2017

why ctor if i may ask? unless one comes from C# it will cause confusion. confusion leads to bugs.
why not constructor? it's unmistakeable!

Name ctor comes from how Solidity encodes call to the constructor. https://github.com/paritytech/pwasm-abi/blob/master/derive/src/lib.rs#L95

@snd
Copy link
Contributor Author

snd commented Nov 13, 2017

Name ctor comes from how Solidity encodes call to the constructor. https://github.com/paritytech/pwasm-abi/blob/master/derive/src/lib.rs#L95

isn't that an implementation detail that shouldn't affect the public API?

@NikVolf
Copy link
Contributor

NikVolf commented Nov 13, 2017

ctor

it has nothing to do with solidity actually, it's just me; i will rename it in abi reimplementation i'm working on now

@snd
Copy link
Contributor Author

snd commented Nov 13, 2017

just looked at https://play.rust-lang.org/?gist=6570adb28cbd3bb1c139e08389a6a12b&version=stable again

in the test at the bottom: having to use explicit blocks and recreating the contract many times because the contract exclusively mutably borrows the context is not the best developer experience.

that's just a minor thing though

@snd
Copy link
Contributor Author

snd commented Nov 13, 2017

@pepyakin really like how your modification removed the need for lifetime params for the real (non testing) case

@snd
Copy link
Contributor Author

snd commented Nov 16, 2017

polkadot uses the concept of an Externalities trait (similar to Context idea) as well:
https://github.com/paritytech/polkadot/blob/2ae67514569af4c1c55f19692bf2dd84307af78c/state_machine/src/lib.rs#L164

maybe we can get some useful ideas from their code

i like the word Externalities

@NikVolf
Copy link
Contributor

NikVolf commented Nov 16, 2017

@snd, it has little to do with context and tests, it's how runtime works

parity has the same "externalities" (and with much more functions) for both evm and wasm
https://github.com/paritytech/parity/blob/master/ethcore/src/externalities.rs

@snd
Copy link
Contributor Author

snd commented Nov 16, 2017

it has quite a bit to do with context in the sense that the context idea and externalities both model abilities of an execution environment (and those environments are even similar) through a trait. so design decisions in one could be useful in the other

@lexfrl
Copy link
Contributor

lexfrl commented Nov 16, 2017

We have a current limitation with a "context" approach:
currently we cant set that context on Client, cause in Client we directly use ext::call
https://github.com/paritytech/pwasm-token-example/blob/master/src/token.rs#L49

@cyberbono3
Copy link

Guys, run failed with a bunch of errors

@openethereum openethereum deleted a comment from lexfrl Mar 7, 2019
@NikVolf
Copy link
Contributor

NikVolf commented Mar 7, 2019

@cyberbono3

fixed for the latest rust version

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants