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

Update ADR8 #4631

Merged
merged 15 commits into from
Sep 19, 2023
Merged

Update ADR8 #4631

merged 15 commits into from
Sep 19, 2023

Conversation

AdityaSripal
Copy link
Member

Description

closes: #XXXX

Commit Message / Changelog Entry

type: commit message

see the guidelines for commit messages. (view raw markdown for examples)


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 and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/).
  • Added relevant godoc comments.
  • Provide a commit message to be used for the changelog entry in the PR description for review.
  • Re-reviewed Files changed in the Github PR explorer.
  • Review Codecov Report in the comment section below once CI passes.

Comment on lines 237 to 241
The callback will then be routed through the callback keeper which will either panic or return a result (success or failure). In the event of a panic or an error, the callback state changes
are discarded and the transaction is committed.

If the transaction runs out of gas because the relayer-defined limit was exceeded the entire transaction is reverted to allow for resubmission. If the chain-defined or user-defined gas limit is reached,
the callback state changes are reverted and the transaction is committed.
Copy link
Member Author

Choose a reason for hiding this comment

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

The panic/revert/commit behavior is the part I need double checked specifically. @srdtrk

@srdtrk srdtrk added docs Improvements or additions to documentation callbacks middleware Issues for callbacks middleware adr8 labels Sep 12, 2023
@srdtrk srdtrk self-assigned this Sep 12, 2023
Copy link
Member

@srdtrk srdtrk left a comment

Choose a reason for hiding this comment

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

Thank you @AdityaSripal, left some questions and suggestions.

docs/architecture/adr-008-app-caller-cbs.md Outdated Show resolved Hide resolved
docs/architecture/adr-008-app-caller-cbs.md Outdated Show resolved Hide resolved
docs/architecture/adr-008-app-caller-cbs.md Outdated Show resolved Hide resolved
docs/architecture/adr-008-app-caller-cbs.md Outdated Show resolved Hide resolved
docs/architecture/adr-008-app-caller-cbs.md Outdated Show resolved Hide resolved
docs/architecture/adr-008-app-caller-cbs.md Show resolved Hide resolved
docs/architecture/adr-008-app-caller-cbs.md Outdated Show resolved Hide resolved
docs/architecture/adr-008-app-caller-cbs.md Outdated Show resolved Hide resolved
@AdityaSripal AdityaSripal requested a review from srdtrk September 14, 2023 14:05
Copy link
Member

@srdtrk srdtrk left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you so much. Just left a couple more nits

docs/architecture/adr-008-app-caller-cbs.md Outdated Show resolved Hide resolved
docs/architecture/adr-008-app-caller-cbs.md Outdated Show resolved Hide resolved
docs/architecture/adr-008-app-caller-cbs.md Show resolved Hide resolved
srdtrk and others added 2 commits September 18, 2023 15:10
Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>
Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Quick skim, looks good. LGTM

Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

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

reads good to me, left minor typo suggestions

docs/architecture/adr-008-app-caller-cbs.md Outdated Show resolved Hide resolved
docs/architecture/adr-008-app-caller-cbs.md Outdated Show resolved Hide resolved
docs/architecture/adr-008-app-caller-cbs.md Outdated Show resolved Hide resolved
docs/architecture/adr-008-app-caller-cbs.md Outdated Show resolved Hide resolved
docs/architecture/adr-008-app-caller-cbs.md Outdated Show resolved Hide resolved
AdityaSripal and others added 2 commits September 19, 2023 12:33
Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com>
Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>
@srdtrk srdtrk merged commit 68f7ab3 into main Sep 19, 2023
20 checks passed
@srdtrk srdtrk deleted the aditya/adr-8-update branch September 19, 2023 11:47
mergify bot pushed a commit that referenced this pull request Sep 19, 2023
* update adr8 to latest implementation decisions

* fix link

* fixed relationship between CallbackPacketData and packet data interfaces

* update changelog

* Apply suggestions from code review

Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>

* fix(docs): fix broken link

* address rest of comments

* Update docs/architecture/adr-008-app-caller-cbs.md

Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com>
Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>

---------

Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>
Co-authored-by: srdtrk <srdtrk@hotmail.com>
Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com>
(cherry picked from commit 68f7ab3)

# Conflicts:
#	docs/architecture/README.md
@mergify mergify bot mentioned this pull request Sep 19, 2023
DimitrisJim pushed a commit that referenced this pull request Sep 20, 2023
* docs: update ADR8 (#4631)

* update adr8 to latest implementation decisions

* fix link

* fixed relationship between CallbackPacketData and packet data interfaces

* update changelog

* Apply suggestions from code review

Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>

* fix(docs): fix broken link

* address rest of comments

* Update docs/architecture/adr-008-app-caller-cbs.md

Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com>
Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>

---------

Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>
Co-authored-by: srdtrk <srdtrk@hotmail.com>
Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com>
(cherry picked from commit 68f7ab3)

# Conflicts:
#	docs/architecture/README.md

* Update README.md

---------

Co-authored-by: Aditya <adityasripal@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adr8 callbacks middleware Issues for callbacks middleware docs Improvements or additions to documentation
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants