docs: RFC 001 tx validation

[tac0turtle]

Description

This pr is meant to capture a conversation in slack and propose it to the community. The updated flow reduces boilerplate code and decreases the amount of duplicated validations happening.

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[julienrbrt]

Are the Discussions and References sections expected to be empty?

[likhita-809]

nice detailing

[tac0turtle]

Are the Discussions and References sections expected to be empty?

thinking what i should reference. I have one or two links in mind for references. For discussion, the goal is to move ant discussions in the pr to that section. Ill wait to see what people think

[aaronc]

I think we should still process ValidateBasic if it's defined (using an interface check) but remove usage in the sdk and move everything to the MsgServer.

[alexanderbez]

I recommend following Hashicorp's RFC template -> https://works.hashicorp.com/articles/rfc-template

There is no predefined structure to the body. Just a background and then whatever else you want. I think we should follow the table section.

[tac0turtle]

I recommend following Hashicorp's RFC template -> https://works.hashicorp.com/articles/rfc-template

There is no predefined structure to the body. Just a background and then whatever else you want. I think we should follow the table section.

thanks for this, ill adjust the template!

[yihuang]

does that mean the tx need to be executed when entering mempool, will that introduce DoS vulnerability to mempool? Or the msgs are not checked at all in mempool, only verify tx wrapping structure like signatures and fees?

[tac0turtle]

there wouldnt be an introduction of another way to run this before entering the mempool. ideally the application mempool can run its own checks in parallel. This way the tx wouldnt enter the mempool and the tx would be removed from comets mempool after a recheck.

In the future it would be ideal if comet asked the application which txs it would like to broadcast so that the app can run these check more smoothly

[tac0turtle]

I think we should still process ValidateBasic if it's defined (using an interface check) but remove usage in the sdk and move everything to the MsgServer.

yes that is the idea initially. When we get to the point of sdk.Msg deprecation then we can consider an extension interface

[colin-axner]

Is there discussion of why ValidateBasic was chosen as a design pattern originally for the SDK? I understand there is likely no original public discussion of it, but it might be useful to retroactively discuss what benefits its existence provides. This commit is the only historical trace I could find.

I have concerns with this direction. I think not including ValidateBasic in the sdk.Msg interface is a valid design decision, if it is the original choice. I don't find ValidateBasic or sdk.Msg to be problematic in any way and I'm not sure why we are changing fundamental design patterns? This change appears to be introducing additional complexity/context when there is likely a non-breaking alternative which doesn't require redefining fundamental design patterns of the SDK. Is there a reason why the msg.ValidateBasic cannot be called in a msg server interceptor?

While this RFC indicates that removal of ValidateBasic is backwards compatible, I find this problematic. It introduces additional context and/or required changes in order to avoid pitfalls. Any caller of the msg server needs to also support this backwards compatible approach of calling ValidateBasic on msgs which implement this interface. We would need to add this to ICA and presumably authz/gov? Removing this backwards compatible support is quite difficult since there is no programmatic way of telling users that they must finalize their migration to stateless checks being called directly in the msg handler.

Users also need to be aware not to change the function naming of ValidateBasic to any other function name to ensure that essential checks are still run. This is additional implicit context that can be easily forgotten or overlooked, thus developers will likely need to make these changes regardless (rendering the backwards compatible support not so useful)

@DimitrisJim pointed out a nice benefit of ValidateBasic to me. It is a strong reminder to developers to do stateless validation of the arguments they are receiving. It seems quite possible to me that new developers could easily forget to check that fields are non-empty/valid. Has there been discussion on this point?

Can we add an explanation on the positives of this proposal and why they outweigh the negatives? I'd like to push back on refactoring when there is no particular reason to do so

[tac0turtle]

I have concerns with this direction. I think not including ValidateBasic in the sdk.Msg interface is a valid design decision, if it is the original choice. I don't find ValidateBasic or sdk.Msg to be problematic in any way and I'm not sure why we are changing fundamental design patterns? This change appears to be introducing additional complexity/context when there is likely a non-breaking alternative which doesn't require redefining fundamental design patterns of the SDK. Is there a reason why the msg.ValidateBasic cannot be called in a msg server interceptor?

i would argue the opposite this simplifies the current approach. If we look at any other ecosystem they do not have this sort of check it is handled when the execution happens. the current design pattern was chosen due do limitations construed when this was written but as the ecosystem evolved we see that this isnt the case and the state less validation doesnt add much value. You can still use validate basic it wasnt removed and wont be removed, but it will be removed from sdk.Message as we are trying to remove global interface registries from the sdk for a few reasons.

While this RFC indicates that removal of ValidateBasic is backwards compatible, I find this problematic. It introduces additional context and/or required changes in order to avoid pitfalls. Any caller of the msg server needs to also support this backwards compatible approach of calling ValidateBasic on msgs which implement this interface. We would need to add this to ICA and presumably authz/gov? Removing this backwards compatible support is quite difficult since there is no programmatic way of telling users that they must finalize their migration to stateless checks being called directly in the msg handler.

users will get an error that msg.Validatebasic doesnt exist on sdk.message and they should remove it or update it to the extension interface. In your comment it seems you are missing the extension interface, if a message implements the extension interface, i.e. doesnt change anything in their repo then it will still work.

pointed out a nice benefit of ValidateBasic to me. It is a strong reminder to developers to do stateless validation of the arguments they are receiving. It seems quite possible to me that new developers could easily forget to check that fields are non-empty/valid. Has there been discussion on this point?

these checks should and can happen at the message server level, it is actually safer to do it this way because modules now dont assume validation is handled for them via validate basic like some teams have done but instead then can rely on it checking the validation with certainty. How is this less safe?

Can we add an explanation on the positives of this proposal and why they outweigh the negatives? I'd like to push back on refactoring when there is no particular reason to do so

happy to have a longer chat about this but sdk.Msg will be deprecated in the near future for a few reasons. Validatebasic in the current form prevents removal of globalbech32 as well. Users can still implement the extension interface if they would like but it is recommended all checks happen in the msg server to make the system less complex and safer