CosmWasm / wasmd

Basic cosmos-sdk app with web assembly smart contracts
Other
368 stars 400 forks source link

MsgInstantiateContract doesn't return the data field from the contract #134

Closed ethanfrey closed 4 years ago

ethanfrey commented 4 years ago

Summary of Bug

res.Data (from sdk.Result) is always set to the contract address. This was a nice way to show the user the address they can call. We now return and parse the same info from the events. Thus, we could return the original data field from the cosmwasm contract, just as we do in Execute.

However, this does make it a bit more unwieldly to get the new address and we will have to upload various caller code, including the client.

Version

all versions from 0.6.0 -> 0.8.0

Steps to Reproduce


For Admin Use

ethanfrey commented 4 years ago

@webmaster128 this is what you requested I believe. After looking at the code and how this address (from res.Data) is used in several places in go code I am a bit less sure about this change. Can you add a convincing argument?

webmaster128 commented 4 years ago

I don't think there is any natural answer to the question what MsgInstantiateContract's result data is supposed to be. Returning the address makes sense as well. I think it is fine to leave it like that but then we need to remove the field in

pub struct InitResponse<T = Never>
where
    T: Clone + fmt::Debug + PartialEq + JsonSchema,
{
    // let's make the positive case a struct, it contrains Msg: {...}, but also Data, Log, maybe later Events, etc.
    pub messages: Vec<CosmosMsg<T>>,
    pub log: Vec<LogAttribute>, // abci defines this as string
    pub data: Option<Binary>,   // abci defines this as bytes
}

I don't need the field there. I was just using it to get some test data. But I can use handle as well.

ethanfrey commented 4 years ago

Good point, the two definitely need to be in sync. And we would finally have a reason why InitResponse != HandleResponse

If you are fine with punting that to 0.9 and using handle for testing we can think of a proper (maybe breaking) solution.

ethanfrey commented 4 years ago

If this is not 0.8.x, then I can tag 0.8.1 wasmd today and merge 0.9 to master (nudge, nudge).

webmaster128 commented 4 years ago

Good, let's do it that way: keep the bug (i.e. the lost InitResponse.data) as is for 8.x and remove the field in 0.9. Closing here.