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

docs: ics29 middleware usage for app devs #1567

Merged
merged 10 commits into from
Jun 23, 2022

Conversation

damiannolan
Copy link
Member

Description

  • About middleware application stacks
  • How to configure an application stack composed of transfer and 29-fee
  • How to configure an application stack composed of 27-interchain-accounts and 29-fee with associated auth module.

ref: #1408


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

### Transfer

See below for an example of how to create an application stack using `transfer` and `29-fee`.
The in-line comments describe the execution flow of packets between the application stack and IBC core.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a little bit confusing that you mention app.go two sections above but only show the code snippet here -- maybe either move the sentence from above "For Cosmos SDK chains this setup...." down to the start of this section or move the code snippet up somewhere closer, and indicate that the code snippet is from app.go

Copy link
Contributor

@charleenfei charleenfei Jun 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also maybe mention a bit about the ordering of the middleware and how it will affect how the code is called (the bug from evmos)

-- this can also go into the main middleware docs but i dont think it hurts to repeat

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meant to leave a reply here earlier... I added an extra sentence to each example section reiterating the app/app.go direction. I feel the ordering is covered by the in line comments in the code snippets and should be covered in the middleware docs which are listed as {prereq} reading.

We may want to review the middleware developer/integration docs also to make sure they are up to date!


# Integration

Learn how to configure the Fee Middleware module with IBC applications. The following document only applies for Cosmos SDK chains. {synopsis}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe be explicit somewhere about who these docs are for -- cosmos SDK devs

Copy link
Contributor

@seantking seantking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean. Nice work.

Copy link
Contributor

@charleenfei charleenfei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work!

@damiannolan damiannolan enabled auto-merge (squash) June 23, 2022 14:29
@damiannolan damiannolan merged commit 5467300 into main Jun 23, 2022
@damiannolan damiannolan deleted the damian/1408-mw-usage-for-app-devs branch June 23, 2022 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants