Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MsgInstantiateContractResponse.data #385

Closed
webmaster128 opened this issue Jan 23, 2021 · 9 comments · Fixed by #427
Closed

Add MsgInstantiateContractResponse.data #385

webmaster128 opened this issue Jan 23, 2021 · 9 comments · Fixed by #427
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@webmaster128
Copy link
Member

MsgExecuteContractResponse and MsgMigrateContractResponse have a contract defined data field. MsgInstantiateContractResponse does not have this for historic reasons (we returned the raw contract address before).

Now that we can easily return structured proto messages, it would be good to add MsgInstantiateContractResponse.data for consistency.

@ethanfrey
Copy link
Member

Ah, so we can have both fields, sender address and contract data in the same response.

Yes, this makes sense. 👍 for being able to return structured data

@alpe alpe added this to the v0.15.0 milestone Jan 25, 2021
@alpe alpe added the enhancement New feature or request label Jan 29, 2021
@alpe alpe modified the milestones: v0.15.0, v0.16.0 Jan 29, 2021
@alpe alpe self-assigned this Mar 3, 2021
@alpe
Copy link
Contributor

alpe commented Mar 3, 2021

Good 💡 The field is easy to add.

As a note: Instantiate can also be called by a gov proposal. The data field would not be returned or printed anywhere.
If there is a requirement for the data then the best we can do IMHO would be adding an attribute to the event.
WDYT? (This also affects migrate proposals) @webmaster128 @ethanfrey

@webmaster128
Copy link
Member Author

As a note: Instantiate can also be called by a gov proposal. The data field would not be returned or printed anywhere.

How does this work for other message types that are executed as part of a gov proposal?

@ethanfrey
Copy link
Member

Hmmm... it makes sense to return it as a hex/base64 encoded attribute as well.

The types allow you to pass in bytes but much code blindly converts to and from string, so let's keep it utf8

@webmaster128
Copy link
Member Author

The types allow you to pass in bytes but much code blindly converts to and from string, so let's keep it utf8

This does not sound like a winning strategy. With the Stargate upgrade it is encouraged to store a serialized protobuf response in there, which is definitely not UTF8.

@alpe
Copy link
Contributor

alpe commented Mar 3, 2021

@webmaster128 do you have found an example or doc you can point me too?

@ethanfrey
Copy link
Member

@webmaster128 @alpe Please read the discussion from here on: cosmos/cosmos-sdk#8624 (comment)

There was quite some discussion about using raw bytes in events

@alpe
Copy link
Contributor

alpe commented Mar 3, 2021

Thanks for the link! They ended up with hex encoded protobuf: cosmos/cosmos-sdk@cb28f10#diff-d2a466b18c54af68dfa1c635b309ebb1c5fe10f9c23fc99d89dcc83c95638ac6R103
Everybody ok with this? 👍 👎
It feels a bit odd to create a new proto type just for the byte data but 🤷

Next question would be to always store the data or only for operations not triggered by a client (gov/contracts) ?
Always would be easier to implement.

@ethanfrey
Copy link
Member

Next question would be to always store the data or only for operations not triggered by a client (gov/contracts) ?
Always would be easier to implement.

I'd take the simpler route

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants