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.

Sign up for GitHub

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

Jump to bottom

Initial ADR for IBC client recovery mechanisms #6488

Merged
merged 10 commits into from
Aug 10, 2020
Merged

Initial ADR for IBC client recovery mechanisms #6488

merged 10 commits into from
Aug 10, 2020

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Jun 23, 2020 •
edited
Loading

Ref cosmos/ibc#421

  • 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

@cwgoes cwgoes added S:proposed x/ibc T: ADR An issue or PR relating to an architectural decision record labels Jun 23, 2020
colin-axner
colin-axner reviewed Jun 23, 2020
View reviewed changes
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

LGTM, left one comment on handling the updating of the client after the propsoal

docs/architecture/adr-026-ibc-client-recovery-mechanisms.md Show resolved Hide resolved
fedekunze
fedekunze previously requested changes Jun 23, 2020
View reviewed changes
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM. The only comment is to add the default values for the flags and specify the unbending period

docs/architecture/adr-026-ibc-client-recovery-mechanisms.md Outdated Show resolved Hide resolved
docs/architecture/adr-026-ibc-client-recovery-mechanisms.md Outdated Show resolved Hide resolved
docs/architecture/adr-026-ibc-client-recovery-mechanisms.md Outdated Show resolved Hide resolved
@cwgoes
Copy link
Contributor Author

cwgoes commented Jun 23, 2020 •
edited
Loading

To be clarified: how this will interact with potential upgrades (e.g. to a different chain ID). Should we allow this?

Consensus: yes.

Will there be a separate period for the backup upgrade path?

Consensus: no.

@cwgoes
Copy link
Contributor Author

cwgoes commented Jun 23, 2020

Maybe #6378 should also be in this ADR.

@cwgoes
Copy link
Contributor Author

cwgoes commented Jun 25, 2020

Spec issue for epoch number - cosmos/ibc#439 - will wait for spec updates before finalising this ADR.

@cwgoes cwgoes mentioned this pull request Jul 6, 2020
Closed
@fedekunze fedekunze added S:blocked Status: Blocked and removed S:proposed labels Jul 30, 2020
@fedekunze fedekunze added this to the IBC 1.0 milestone Jul 30, 2020
@cwgoes cwgoes dismissed fedekunze’s stale review August 6, 2020 14:58

Comments addressed.

@cwgoes cwgoes removed the S:blocked Status: Blocked label Aug 6, 2020
colin-axner
colin-axner approved these changes Aug 10, 2020
View reviewed changes
Copy link
Contributor

@colin-axner colin-axner left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. From an implementation point of view, I suppose all we really need in 02-client is to add

Unfreeze() error to ClientState interface.

Then clients can implement arbitrary conditions for unfreezing. It should just be well specified so proposals don't pass that just result in an error.

fedekunze
fedekunze approved these changes Aug 10, 2020
View reviewed changes
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

👍

@cwgoes cwgoes merged commit 769387b into master Aug 10, 2020
@cwgoes cwgoes deleted the cwgoes/architecture-doc-ibc-recovery branch August 10, 2020 16:19
@cwgoes cwgoes mentioned this pull request Aug 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AdityaSripal AdityaSripal AdityaSripal left review comments

@milosevic milosevic milosevic left review comments

@fedekunze fedekunze fedekunze approved these changes

@colin-axner colin-axner colin-axner approved these changes

@aaronc aaronc Awaiting requested review from aaronc

@alessio alessio Awaiting requested review from alessio

@alexanderbez alexanderbez Awaiting requested review from alexanderbez

@clevinson clevinson Awaiting requested review from clevinson

@jgimeno jgimeno Awaiting requested review from jgimeno

Assignees
No one assigned
Labels
T: ADR An issue or PR relating to an architectural decision record
Projects
None yet
Milestone
IBC 1.0 [Implementation]
Development

Successfully merging this pull request may close these issues.
5 participants
@cwgoes @milosevic @AdityaSripal @colin-axner @fedekunze