Check on contract_data set method to avoid overflow on contract data set

[heytdep]

What problem does your feature solve?

Assuming we have an enum with data names to be added to a contract's data as a u32 primitive, like in the following implementation:

#[derive(Clone, Copy)]
#[repr(u32)]
pub enum DataName {
    Admin = 0,
    AllowedToken = 1,
    Nonce = 2
}

When implementing IntoVal for that enum, the into_val method is called on the enum. The ideal implementation (as showed in the examples) would be the following:

impl IntoVal<Env, RawVal> for DataName {
    fn into_val(self, env: &Env) -> RawVal {
        (self as u32).into_val(env)
    }
}

However, it might happen to forget adding as u32 (at least, it happened to me :) ). This might result in setting contract data with an enum that will overflow the stack:

What would you like to see?

As you can see from the screenshot, the error message is not really descriptive since it's not a panic. I think there should be a check before passing overflowed values in the contract data, and if it happens, catch the overflow and return a panic describing the error.

What alternatives are there?

Maybe the into_val method can directly perform the overflow check?

[jonjove]

@heytdep thanks for the report, we'll look into this and see what we can do to make it safer and easier to use.

[sisuresh]

Hi @heytdep, this error isn't an integer overflow. It's a stack overflow due to a recursive call with no conditional return. Without the as u32, DataName::into_val will just keep calling into itself. I get a warning below in visual studio code with rust-analyzer. Were you able to see this warning?

function cannot return without recursing
`#[warn(unconditional_recursion)]` on by default

[heytdep]

Hi @sisuresh thanks for the response, yes that's for sure not an integer overflow, just saw the warning when compiling again. my concern was about the impact of this if it gets uploaded on-chain since it seems that the error happens at runtime (?) How would this stack error be handled since contracts aren't executed in isolation? Anyways, closing since it seems that there's nothing that can be done.

[sisuresh]

That's a good question @heytdep. Each contract call will run in a WASM VM on the network, so that specific VM would fail. Our metering infrastructure (which is still being worked on) should stop execution once the budget runs out. I believe we'll also have a maximum amount of resources a contract call can use, which should keep the network safe from contracts with unconditional recursion. @jayz22 please correct me if I'm wrong.

[leighmcculloch]

We should try and eliminate the need to implement IntoVal for integer enums to remove the need to write this implementation by hand. We have an issue open for that:

• Contract Spec: Add support for integer enums #217

[jonjove]

@leighmcculloch I've also been questioning whether the domain should even be an integer enum. Perhaps it should just be a Symbol.

We should still support integer enums, I just don't know if they should be used in this context.

[leighmcculloch]

@jonjove In the case of the domain, could the domain be replaced by the name of the function?

[leighmcculloch]

Because I'm thinking that it would probably be beneficial for every signed payload to include the following:

• network passphrase
• contract id
• function name

[jonjove]

@leighmcculloch that's exactly why I was thinking of changing to a Symbol, to make it the function name. It's worth noting we shouldn't force it to be the function name, since I can imagine cases where you want to sign some kind of attestation that's not associated directly with a function.

And yes, 100% agreed that network passphrase (or hash thereof) and contract ID should be in every signed message.

[leighmcculloch]

we shouldn't force it to be the function name

+1, this is all flexible and people can modify the approach in many ways, but as a default pattern we present and demonstrate a Symbol containing a fn name feels simple and appropriate.

We don't need the Symbol to be included in the request parameters since it can be derived from the fn name. I don't think leaving it out of the request parameters harms the intuitive-ness of the approach.