Skip to content

docs: address GMP review comments (FOU-1386, FOU-1392) - #1400

Merged
srdtrk merged 3 commits into
docsfrom
serdar/fou-1386-gmp-docs-review-fixes
Aug 20, 2026
Merged

docs: address GMP review comments (FOU-1386, FOU-1392)#1400
srdtrk merged 3 commits into
docsfrom
serdar/fou-1386-gmp-docs-review-fixes

Conversation

@srdtrk

@srdtrk srdtrk commented Aug 20, 2026

Copy link
Copy Markdown
Member

🤖 Gnut (automated assistant posting on behalf of Serdar)

Addresses the 8 inline comments I left in review #1380 (review) on the two GMP pages (FOU-1386, FOU-1392). Based on docs, not main, so it lands as a follow-up on top of #1380's branch.

Two commits, one per page/ticket, so they can be reviewed separately.

# Comment What changed
1 gmp.md:46 — payload is opaque, must be encoded for the counterparty GMP impl Fixed. The payload bullet and the paragraph now say the payload is opaque to GMP and that encoding it for the destination implementation is the sender's job; for IBC-solidity that is an ABI-encoded Solidity call (selector + ABI args) handed to the target verbatim. Split out the packet data encoding (application/x-solidity-abi) into its own paragraph, which the old text conflated with the payload encoding.
2 gmp.md:80 — mostly packet lifecycle; say the relayer pays gas and insufficient gas can be retried Fixed. Dropped the lifecycle narration (proof verification, router commits and emits the ack) down to the existing /how-ibc-works/packet-lifecycle link, kept the six GMP-specific steps, and added: the relayer delivers and pays, the call runs on whatever gas its transaction leaves, and an attempt that reverts before the packet is received leaves the packet in flight and can be re-delivered with more gas until the timeout. Also moved the gas paragraph out of "What comes back", where it was repeated.
3 ics27-…:36 — flowchart not useful Cut. Flowchart deleted; the "one account per (clientId, sender, salt)" line it sat above is kept.
4 ics27-…:72 — diagram too complex, split it and box each chain Fixed. The one 8-participant diagram is now three sequenceDiagrams — send, receive-and-execute, acknowledge — each paired with its own paragraph and using box Chain A … end / box Chain B … end so the chain name is no longer repeated in every participant label.
5, 6 ics27-…:140 — title doesn't say what the section is; ics27-…:160 — not sure this section is needed Cut. "Packet handlers the router calls" is gone: its handler table restated "How a call moves through the contracts" and its closing paragraph restated "Acknowledgements and sender callbacks". The two things only it said — the three onRecvPacket checks, and that the router alone may call the three handlers — moved up into "How a call moves through the contracts". If you'd rather keep the section under a clearer title, say so and I'll restore it.
7 ics27-…:176 — note that empty salt is allowed Fixed. Added that an empty salt is allowed and gives the sender one default account per client, which matches what gmp.md already said. sendCall only rejects an empty payload (ICS27PayloadEmpty); salt length is never checked and is hashed as-is into the identifier.
8 ics27-…:247 — "this is actually a bug in ICS27GMP if true" No doc change — deliberately. Both copies of that paragraph (ics27-gmp-and-accounts.md and gmp.md) are left byte-identical and untouched, pending the answer to the bug question. If the FailedCall / empty-revert-data reasoning is wrong, both need rewriting together. Item 2's new wording is scoped to attempts that fail before the packet is received, so it doesn't contradict either copy whichever way that lands.

Net effect: gmp.md +7/−7, ics27-gmp-and-accounts.md +42/−51 (one flowchart and one ~20-line section gone, one diagram split into three smaller ones).

Two things worth a look:

  • box in sequenceDiagram is standard mermaid ≥9.4, but no other page in this docs set uses it, so it hasn't been confirmed against Mintlify's renderer here. If it doesn't render, the fallback is short participant aliases plus a Note over per chain.
  • No other files are touched, no link paths changed, and no claim was altered beyond the seven items above.

Draft on purpose — marking it ready is @evan's call.

srdtrk added 2 commits August 20, 2026 16:05
Signed-off-by: srdtrk <srdtrk@hotmail.com>
…392)

Signed-off-by: srdtrk <srdtrk@hotmail.com>
@linear-code

linear-code Bot commented Aug 20, 2026

Copy link
Copy Markdown

FOU-1386

FOU-1392

…ivation

The delivery paragraph on the GMP page said a failed attempt could be
re-delivered with more gas, which reads as the general rule for an
out-of-gas destination call and contradicts the FailedCall paragraph
later on the same page. ICS26Router.recvPacket writes the packet receipt
before it calls the application, so past proof verification the packet is
consumed either way; only a delivery that reverts before the receipt is
written leaves it in flight. Say that instead.

Also restore two details dropped with the old packet-handlers section:
how ICS27GMP derives the callback's success flag, and that the three
router handlers revert ICS27Unauthorized for any other caller.

Signed-off-by: srdtrk <srdtrk@hotmail.com>
@srdtrk
srdtrk marked this pull request as ready for review August 20, 2026 15:01
@srdtrk
srdtrk requested a review from a team as a code owner August 20, 2026 15:01
@srdtrk
srdtrk merged commit 82735e9 into docs Aug 20, 2026
8 of 9 checks passed
@srdtrk
srdtrk deleted the serdar/fou-1386-gmp-docs-review-fixes branch August 20, 2026 15:02
@greptile-apps

greptile-apps Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR refines the GMP documentation following review feedback.

  • Clarifies the distinction between opaque destination-call payload encoding and packet encoding.
  • Explains relayer gas responsibility and retry behavior around packet receipt.
  • Simplifies the ICS27 GMP contract-flow documentation by splitting its sequence diagram and removing duplicated handler material.
  • Documents empty salts and acknowledgement success classification.

Confidence Score: 5/5

The documentation-only PR appears safe to merge with no actionable correctness issue identified.

The revised payload, relayer, receipt, account, and acknowledgement explanations remain consistent with the repository’s documented GMP model, while moved material preserves the existing contracts.

Important Files Changed

Filename Overview
docs/3-applications/gmp.md Clarifies payload encoding, packet execution, gas responsibility, receipt handling, and acknowledgement behavior without an actionable changed-code defect.
docs/5-ibc-solidity-contracts/ics27-gmp-and-accounts.md Reorganizes contract-flow diagrams and handler documentation while adding accurate salt and acknowledgement details.

Reviews (1): Last reviewed commit: "docs: correct the gas-retry boundary and..." | Re-trigger Greptile

dhfang pushed a commit that referenced this pull request Aug 26, 2026
* docs: address GMP review comments on gmp.md (FOU-1386)

Signed-off-by: srdtrk <srdtrk@hotmail.com>

* docs: address GMP review comments on ics27-gmp-and-accounts.md (FOU-1392)

Signed-off-by: srdtrk <srdtrk@hotmail.com>

* docs: correct the gas-retry boundary and restore the success-flag derivation

The delivery paragraph on the GMP page said a failed attempt could be
re-delivered with more gas, which reads as the general rule for an
out-of-gas destination call and contradicts the FailedCall paragraph
later on the same page. ICS26Router.recvPacket writes the packet receipt
before it calls the application, so past proof verification the packet is
consumed either way; only a delivery that reverts before the receipt is
written leaves it in flight. Say that instead.

Also restore two details dropped with the old packet-handlers section:
how ICS27GMP derives the callback's success flag, and that the three
router handlers revert ICS27Unauthorized for any other caller.

Signed-off-by: srdtrk <srdtrk@hotmail.com>

---------

Signed-off-by: srdtrk <srdtrk@hotmail.com>
dhfang added a commit that referenced this pull request Aug 26, 2026
* Add docs

Add IBC docs drafts for review

* update link -> cli

* docs: review fixes for core router and store (FOU-1382)

* docs: IBC Link -> IBC CLI in core router page

* docs: cut two redundant sentences from packet lifecycle (FOU-1385)

* docs: review fixes for ICS26Router page (FOU-1391)

* Update clients-and-counterparties.md

Signed-off-by: dhfang <dhsfang@gmail.com>

* Update packets-and-applications.md

Signed-off-by: dhfang <dhsfang@gmail.com>

* docs: address GMP review comments (FOU-1386, FOU-1392) (#1400)

* docs: address GMP review comments on gmp.md (FOU-1386)

Signed-off-by: srdtrk <srdtrk@hotmail.com>

* docs: address GMP review comments on ics27-gmp-and-accounts.md (FOU-1392)

Signed-off-by: srdtrk <srdtrk@hotmail.com>

* docs: correct the gas-retry boundary and restore the success-flag derivation

The delivery paragraph on the GMP page said a failed attempt could be
re-delivered with more gas, which reads as the general rule for an
out-of-gas destination call and contradicts the FailedCall paragraph
later on the same page. ICS26Router.recvPacket writes the packet receipt
before it calls the application, so past proof verification the packet is
consumed either way; only a delivery that reverts before the receipt is
written leaves it in flight. Say that instead.

Also restore two details dropped with the old packet-handlers section:
how ICS27GMP derives the callback's success flag, and that the three
router handlers revert ICS27Unauthorized for any other caller.

Signed-off-by: srdtrk <srdtrk@hotmail.com>

---------

Signed-off-by: srdtrk <srdtrk@hotmail.com>

* Update relayer.md

* Update relayer.md

Signed-off-by: dhfang <dhsfang@gmail.com>

* Update relayer.md

Signed-off-by: dhfang <dhsfang@gmail.com>

* add minor edis to ift docs (#1408)

* move codeblock

* updates

* update

* docs: add IBC CLI tutorial, guides, and reference (#1419)

* docs: add the IBC CLI page set

* small updates

* updates

* Update 1-overview.md

---------

Co-authored-by: Dennis Fang <dhsfang@gmail.com>

* update links and rename files

* Update README.md

* update links

* update links

* updates

* update links and move GMP guide

* correct the relayer API reference and Go version in the docs

* regenerate the CLI and configuration reference tables from source

* warn against hosting several attestors in one process for security-critical deployments

* tighten the attestor warning and note the manual render-config merge

* add SPDX license headers to the docs pages

Signed-off-by: Dennis Fang <dhsfang@gmail.com>

* scope SPDX headers to source and exclude prose documentation

Signed-off-by: Dennis Fang <dhsfang@gmail.com>

* drop the paths-ignore comment

Signed-off-by: Dennis Fang <dhsfang@gmail.com>

---------

Signed-off-by: dhfang <dhsfang@gmail.com>
Signed-off-by: srdtrk <srdtrk@hotmail.com>
Signed-off-by: Dennis Fang <dhsfang@gmail.com>
Co-authored-by: Gjermund Garaba <gjermund@garaba.net>
Co-authored-by: dhfang <dhsfang@gmail.com>
Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>
Co-authored-by: Dmitry S <11892559+swift1337@users.noreply.github.com>
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 this pull request may close these issues.

1 participant