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

Reply should contain the original payload from the triggering SubMsg #1909

Closed
ewoolsey opened this issue Oct 6, 2023 · 14 comments · Fixed by #2008
Closed

Reply should contain the original payload from the triggering SubMsg #1909

ewoolsey opened this issue Oct 6, 2023 · 14 comments · Fixed by #2008
Milestone

Comments

@ewoolsey
Copy link

ewoolsey commented Oct 6, 2023

Motivation

We want to send a ReplyAlways SubMsg. If and only if that message fails, we want to send a different message with the same context.

The first message has the payload ExecuteMsg::TryThis{ important_context: Context }.
If and only if that message fails, then we want to retry with an alternative ExecuteMsg::TryThisInstead{ same_context: Context }.

Currently there is no way to accomplish this as far as I can tell. If TryThis fails, then the context in which it was called is no longer around in the reply. We could try a hack and store the context to storage before we send either message, but that only works if we are sending one instance of TryThis + maybe TryThisInstead . If we are sending multiple instances then that isn't possible.

@webmaster128
Copy link
Member

Agreed. I think there should be a generic data field (maybe with a better name) that allows any custom data to be communicated from the origin of the submessage to the reply.

but that only works if we are sending one instance of TryThis + maybe TryThisInstead . If we are sending multiple instances then that isn't possible.

Can't you have a Map from submessage ID -> Context to achieve that?

@ewoolsey
Copy link
Author

ewoolsey commented Oct 7, 2023

Agreed. I think there should be a generic data field (maybe with a better name) that allows any custom data to be communicated from the origin of the submessage to the reply.

but that only works if we are sending one instance of TryThis + maybe TryThisInstead . If we are sending multiple instances then that isn't possible.

Can't you have a Map from submessage ID -> Context to achieve that?

In my particular case I’m already using the submsg id to represent the variant of the original calling message.

I think perhaps the best way to solve this problem is the sub message id should just be changed to sub msg data sort of like you were suggesting. I see no reason to keep both fields around.

@webmaster128 webmaster128 added this to the 2.0.0 milestone Jan 25, 2024
@webmaster128
Copy link
Member

webmaster128 commented Jan 25, 2024

I see no reason to keep both fields around.

The advantage of having an id plus a data field is that you can better separate between an ID that describes the high level operations (as in which of those 5 places in the contract emitted the submessage) and a free form payload that you send around and can have a completely different format depending on the ID. For backwards compatibility an addition is also much more straight forward.

I think such a feature can avoid a lot of

  • write to store
  • read
  • delete

flows for cases in which you only need temporary data to be copied from original execution to the reply call.

@ewoolsey
Copy link
Author

Backwards compatibility is the strongest point for keeping it around I think! But yes having a data field would absolutely be incredibly useful.

@webmaster128
Copy link
Member

webmaster128 commented Jan 25, 2024

I think at this point it's just a matter of giving the field the right name. What about memo ;)

Is type Binary fine? Or do you have a different type in mind?

@ewoolsey
Copy link
Author

Yeah I think Binary is the correct choice given that it's already a standard.I actually prefer data as the field name haha.

@webmaster128
Copy link
Member

The issue with "data" is that we have data in SubMsgResponse already which is the data field coming from the Cosmos SDK's message execution. We need something different to avoid this very likely confusion.

@ewoolsey
Copy link
Author

Then I would go with msg or payload. Probably msg

@webmaster128
Copy link
Member

I think we can go with payload which so far has no overlap. I think it's important to keep it general since other contract developers might want to pass over some completely different data than a message.

@webmaster128
Copy link
Member

Is 1 KiB per SubMsg a good limit to avoid abuse and still serve the majority of use cases? 1 KiB in JSON looks like this

{"payload":"1UotRfFGhehoA1qQcexm3+d4vf7nDlBGD/XqZcTlVxXSQFIMeqnfOLpjL231dwcTJCoR08jMIhvVp0TjfcHqHGTrylAmnLnZ6n5ojUFb5Gfc3fcAuf9BXIEEsu2LOjgx4K/+S+oplBZ5ffIxwtbocXuIz9C9eJq50Wz2gfwSQ7vytJFNp9C/WrF0+0Po3SsarjOh1w5u8pQU0+tfVYiDakP/ICI0bpTCGvOpCpizMAL77XNnYaGkdg9VWIGwXUTYuMJHqYZeM1vzTriH9P3eojjo2vgYn3wqeHVtAp8ey93Yu2/w0D+wHfHJT5Gz4g0b5XArINKZ30meFFhr3Hx3B5kT4OG2bsVykDDJNBZ3G5c05l9SAHIX6wF5JpRKNLZjdvKMlqFotP9cqJX/Eajjy39YjvvqfXRb0bn+cI+5z3MOwxNZhopd6USQ4TulwTM7+ns/gR3pPbRAzuKrEAITPiIGELg1iDQRiOh7eVE+tHDy7PfLUTPKIh3Z+Gkj3m5vQZcmgvjWykT+Yqpgct8EI/C4dG1k5lZ/89zGMr4tlERdti6XQDfqaWMdLy315N+GLNtm6k935uvHnJq+2hqTeVcbeCPfU3u3wuNR4d9k+wp2PDVMZZ9FTjqCfcrCVswhz/dK3NQSrDwLw2tR0yqRUBwOYnwoG8I3BJN61DePjP73VfSxwOZqHSunk1PybDJUNT4AFx5fX6TWMQb4QHX/6FNlN1m1STEiNg7YRGPFtYmSDfOEaEYvo/I/D1RO1CPzoZ/DHHCLpmTddW6cqThZ45nXN8Q/ekYLcbUamWgpRz7ouEy9632F38t6N4+wz/Xy19lLDiUAtHZlDOTX7gMqHhm/RXrKvS1kdS2UjVGQ3cpvrwFsXnhUqGwlYcPa9IEz4xfRJgo572pKwkYJiun90D+sv92+7uGbYYLzrrKMM7TgvuPMIJ3MuEaoPNSzgLb0WzEMEKAT065n7sgXH/VdSJIDuQQkQiPYkh8Gn9P3luCqL3Z6vgXxbEsAawGrwy8K8h6peAMM9p9bb9TC41PTPPGLfzw+efUBpen/r2vyplBVY2bhkmn4bocwo2qTdE8oZHsk+/aOl+5x13ab1tTv5vWvXrKKT1RiiUj5dE03gh/T9fGykYdG81oSSIrqbgpv993VOOnR6H/cWHiRN3flT3ScTq8Si9nY+SYmevaM+GjBVCfxmls/G6TfBAaRsqV4T6iarULMeWW8MF/cFy4ZGphqBX8FE0o4JivQfhuH3/GIBs5rzItvy+z7zRGQsw79lM9XoFS5Czd9kIEIvtZSOMpYC0rPmFEVkCwq42Vcpsrdf/SYXsZngiZIW8lR4+qCCEmF/KJA2CrDIBs1D4hhFA=="}

We could leave it up to deserialization gas alone but it probably helps contract devs to know how much data they can assume to work.

@ewoolsey
Copy link
Author

ewoolsey commented Jan 27, 2024

1KiB seems a touch small to me. I think there would be many valid use cases that would exceed that. What are the limits on other messages? Do execute messages have a limit for example?

As a user I would probably expect never to run into a limit here. What abuse do imagine is possible if we we're to increase this limit to something like 10KiB? As long as the callers are still required to pay gas, I don't see the upside in creating a small message size limit.

@webmaster128
Copy link
Member

There is a global read limit on the entire contract output (unparsed) of 64 MiB. This is then further limited by the JSON deserializer gas cost in wasmvm which should kick in much earlier (deault is 1 gas/byte).

Now I'm not conviced if a limit is desirable at all. I was thinking that predictable behaviour on all systems is better than whatever gas meter allows, which can change over time. But I also don't see how a very large field could harm the system.

@webmaster128
Copy link
Member

I think we can just add a generous limit of 128 KiB and leave everything else to the gas meter. This is the same max amount you can write in a database entry with CosmWasm. This should avoid resource abuse attempts that might disrupt the block execution.

@webmaster128
Copy link
Member

Could you check the PR in #2008?

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

Successfully merging a pull request may close this issue.

2 participants