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

Write ADR for IBC handler logic in msg.ValidateBasic #150

Closed
3 tasks
colin-axner opened this issue Apr 29, 2021 · 1 comment
Closed
3 tasks

Write ADR for IBC handler logic in msg.ValidateBasic #150

colin-axner opened this issue Apr 29, 2021 · 1 comment

Comments

@colin-axner
Copy link
Contributor

Summary

see previous discussion

Currently we squeeze as many IBC checks into msg.ValidateBasic as possible. Sometimes we duplicate this logic in the handler, sometimes we don't.

In order to ensure the handler are secure without accepting a msg as an argument, we can either, keep maximizing msg.ValidateBasic to save transaction gas and duplicate all the necessary checks or we can move all necessary IBC logic out of message ValidateBasic and into core IBC.

Historically IBC is designed to be as minimal and defensive as possible. I'm slightly in favor of removing all core IBC logic from msg.ValidateBasic() and into the handlers. I'm fine with leaving clientState.Validate, misbehaviour.ValidateBasic etc checks in msg.ValidateBasic (and duplicating these) but other checks which are less readable and more specific should be de-duplicated, such as this check

At the very least, I'd like an ADR to be decided upon and implemented such that we can have a clear understanding of what checks are needed where. I do not want to be adding defensive checks in some handlers and not in others because we may or may not have use cases for some and not others. We should be consistent across the board, we should intentionally decide upon some design


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
CosmosCar pushed a commit to caelus-labs/ibc-go that referenced this issue Nov 6, 2023
…#150)

Bumps [github.com/dgraph-io/badger/v3](https://github.com/dgraph-io/badger) from 3.2011.1 to 3.2103.2.
- [Release notes](https://github.com/dgraph-io/badger/releases)
- [Changelog](https://github.com/dgraph-io/badger/blob/v3.2103.2/CHANGELOG.md)
- [Commits](dgraph-io/badger@v3.2011.1...v3.2103.2)

---
updated-dependencies:
- dependency-name: github.com/dgraph-io/badger/v3
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@colin-axner
Copy link
Contributor Author

Going to close this issue as stale. I'm not sure how much benefit it would provide and hasn't been an issue since I opened the issue

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

No branches or pull requests

1 participant