imp: transfer total escrow follow ups #3558

colin-axner · 2023-05-04T09:31:23Z

Description

feature branch containing changes approved in smaller prs:

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.

* add docs for total escrow feature * revert change * fix tag * Update docs/migrations/v7-to-v7_1.md --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

* some more review comments * Rename pathAndEscrowAMount to pathAndEscrowAmount --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>

* add invariant for total escrow * address review comment * refactor: simplify logic for asserting invariant * fix: use safeAdd instead of append * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Damian Nolan <damiannolan@gmail.com> --------- Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com>

# Conflicts: # modules/apps/transfer/keeper/migrations_test.go # modules/apps/transfer/keeper/relay_test.go

codecov-commenter · 2023-05-15T20:07:15Z

Codecov Report

Merging #3558 (decbade) into main (ccfce10) will increase coverage by 0.03%.
The diff coverage is 90.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3558      +/-   ##
==========================================
+ Coverage   78.76%   78.80%   +0.03%     
==========================================
  Files         186      187       +1     
  Lines       12816    12847      +31     
==========================================
+ Hits        10095    10124      +29     
- Misses       2289     2292       +3     
+ Partials      432      431       -1

Impacted Files	Coverage Δ
modules/apps/transfer/keeper/invariants.go	`85.71% <85.71%> (ø)`
modules/apps/transfer/keeper/keeper.go	`90.47% <100.00%> (+1.85%)`	⬆️
modules/apps/transfer/module.go	`52.63% <100.00%> (ø)`

* do not store 0 escrow amout * adapt success test * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com> * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Damian Nolan <damiannolan@gmail.com> * add comments --------- Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com>

…ry the endpoint in e2e (#3605)

damiannolan · 2023-05-24T11:51:53Z

e2e/testvalues/values.go

One more to add here after conflicts are resolved #3553 (comment)

damiannolan

Everything in this PR looks good to me! Squeaky clean 🧽

Linter is failing but its likely trivial, maybe it could be the var block thing I mentioned 🤔

e2e/testvalues/values.go

damiannolan · 2023-05-25T07:28:56Z

e2e/testvalues/values.go

we could use a var block to declare all feature releases but I think having them separate is also nice and readable!

docs/apps/transfer/client.md

DimitrisJim · 2023-05-25T07:58:19Z

docs/migrations/v7-to-v7_1.md

-[For more information please refer to the 09-localhost light client module documentation](../ibc/light-clients/localhost/overview.md).
+### Transfer migration
+
+An automatic migration handler (TODO: add link after backport to v7.1.x is merged) is configured in the transfer module to set the total amount in escrow for all denominations of coins that have been sent out. For each denomination a state entry is added with the total amount of coins in escrow regardless of the channel from which they were transferred.


just dropping a comment here to make sure this TODO is not forgotten

Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com>

* docs: add adr 011 for total escrow state entry * indentation * code formatting * Apply suggestions from code review Co-authored-by: Damian Nolan <damiannolan@gmail.com> * Apply suggestions from code review Co-authored-by: Charly <charly@interchain.io> --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: Charly <charly@interchain.io>

colin-axner

ACK, LGTM

damiannolan

ayy! ACK latest commits since review. LGTM 🚀

* transfer (total escrow): add documentation and migration docs (#3509) * add docs for total escrow feature * revert change * fix tag * Update docs/migrations/v7-to-v7_1.md --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * transfer (total escrow): some more review comments (#3519) * some more review comments * Rename pathAndEscrowAMount to pathAndEscrowAmount --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> * transfer (total escrow): add invariant for total escrow (#3506) * add invariant for total escrow * address review comment * refactor: simplify logic for asserting invariant * fix: use safeAdd instead of append * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Damian Nolan <damiannolan@gmail.com> --------- Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> * imp: do not store total escrow when amount is zero (#3585) * do not store 0 escrow amout * adapt success test * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com> * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Damian Nolan <damiannolan@gmail.com> * add comments --------- Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> * add feature release for total escrow state entry to conditionally query the endpoint in e2e (#3605) * fix total escrow cli documentation * Apply suggestions from code review Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> * address review comments * docs: adr 011 total escrow state entry (#3641) * docs: add adr 011 for total escrow state entry * indentation * code formatting * Apply suggestions from code review Co-authored-by: Damian Nolan <damiannolan@gmail.com> * Apply suggestions from code review Co-authored-by: Charly <charly@interchain.io> --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: Charly <charly@interchain.io> --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: Charly <charly@interchain.io> (cherry picked from commit 6f628d9) # Conflicts: # docs/architecture/README.md # docs/migrations/v7-to-v7_1.md # e2e/testconfig/testconfig.go # e2e/tests/transfer/base_test.go

* imp: transfer total escrow follow ups (#3558) * transfer (total escrow): add documentation and migration docs (#3509) * add docs for total escrow feature * revert change * fix tag * Update docs/migrations/v7-to-v7_1.md --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * transfer (total escrow): some more review comments (#3519) * some more review comments * Rename pathAndEscrowAMount to pathAndEscrowAmount --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> * transfer (total escrow): add invariant for total escrow (#3506) * add invariant for total escrow * address review comment * refactor: simplify logic for asserting invariant * fix: use safeAdd instead of append * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Damian Nolan <damiannolan@gmail.com> --------- Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> * imp: do not store total escrow when amount is zero (#3585) * do not store 0 escrow amout * adapt success test * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com> * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Damian Nolan <damiannolan@gmail.com> * add comments --------- Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> * add feature release for total escrow state entry to conditionally query the endpoint in e2e (#3605) * fix total escrow cli documentation * Apply suggestions from code review Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> * address review comments * docs: adr 011 total escrow state entry (#3641) * docs: add adr 011 for total escrow state entry * indentation * code formatting * Apply suggestions from code review Co-authored-by: Damian Nolan <damiannolan@gmail.com> * Apply suggestions from code review Co-authored-by: Charly <charly@interchain.io> --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: Charly <charly@interchain.io> --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: Charly <charly@interchain.io> (cherry picked from commit 6f628d9) # Conflicts: # docs/architecture/README.md # docs/migrations/v7-to-v7_1.md # e2e/testconfig/testconfig.go # e2e/tests/transfer/base_test.go * fix conflicts * remove dead links --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

crodriguezvega and others added 3 commits April 27, 2023 18:37

transfer (total escrow): add documentation and migration docs (#3509)

a448960

* add docs for total escrow feature * revert change * fix tag * Update docs/migrations/v7-to-v7_1.md --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

transfer (total escrow): some more review comments (#3519)

9f99eee

* some more review comments * Rename pathAndEscrowAMount to pathAndEscrowAmount --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>

colin-axner added the backport-to-v7.1.x label May 4, 2023

Merge branch 'main' into feat/total-escrow-state-entry

5b6e8b4

# Conflicts: # modules/apps/transfer/keeper/migrations_test.go # modules/apps/transfer/keeper/relay_test.go

crodriguezvega and others added 4 commits May 16, 2023 14:33

Merge branch 'main' into feat/total-escrow-state-entry

28eab1e

add feature release for total escrow state entry to conditionally que…

1065a4d

…ry the endpoint in e2e (#3605)

fix total escrow cli documentation

e5021cd

damiannolan mentioned this pull request May 24, 2023

feat(transfer)!: migrate transfer parameters to be self managed #3553

Merged

14 tasks

damiannolan reviewed May 24, 2023

View reviewed changes

Merge branch 'main' into feat/total-escrow-state-entry

83ccf6f

damiannolan approved these changes May 25, 2023

View reviewed changes

DimitrisJim reviewed May 25, 2023

View reviewed changes

crodriguezvega and others added 2 commits May 25, 2023 10:05

Apply suggestions from code review

7583427

Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com>

Merge branch 'main' into feat/total-escrow-state-entry

cdc3941

crodriguezvega mentioned this pull request May 25, 2023

e2e: conditionally querying total escrow based on release version #3655

Merged

9 tasks

crodriguezvega and others added 3 commits May 25, 2023 15:45

address review comments

997bf2b

Merge branch 'main' into feat/total-escrow-state-entry

a120641

crodriguezvega marked this pull request as ready for review May 25, 2023 15:00

crodriguezvega requested review from AdityaSripal, crodriguezvega, charleenfei, chatton and srdtrk as code owners May 25, 2023 15:00

crodriguezvega and others added 3 commits May 26, 2023 12:20

Merge branch 'main' into feat/total-escrow-state-entry

3830d78

Merge branch 'main' into feat/total-escrow-state-entry

3382717

Merge branch 'main' into feat/total-escrow-state-entry

d5bee91

colin-axner commented May 30, 2023

View reviewed changes

damiannolan approved these changes May 30, 2023

View reviewed changes

DimitrisJim approved these changes May 30, 2023

View reviewed changes

crodriguezvega added 2 commits May 31, 2023 12:00

Merge branch 'main' into feat/total-escrow-state-entry

d8e200d

Merge branch 'main' into feat/total-escrow-state-entry

decbade

crodriguezvega merged commit 6f628d9 into main Jun 1, 2023

crodriguezvega deleted the feat/total-escrow-state-entry branch June 1, 2023 07:05

mergify bot mentioned this pull request Jun 1, 2023

imp: transfer total escrow follow ups (backport #3558) #3718

Merged

crodriguezvega mentioned this pull request Jun 2, 2023

Follow up items for #3019 #3368

Closed

7 tasks

faddat mentioned this pull request Jun 23, 2023

v7.2.0 for sdk 50 #3948

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

imp: transfer total escrow follow ups #3558

imp: transfer total escrow follow ups #3558

colin-axner commented May 4, 2023 •

edited

Loading

codecov-commenter commented May 15, 2023 •

edited

Loading

damiannolan May 24, 2023

damiannolan left a comment •

edited

Loading

damiannolan May 25, 2023

DimitrisJim May 25, 2023

colin-axner left a comment

damiannolan left a comment

imp: transfer total escrow follow ups #3558

imp: transfer total escrow follow ups #3558

Conversation

colin-axner commented May 4, 2023 • edited Loading

Description

Commit Message / Changelog Entry

codecov-commenter commented May 15, 2023 • edited Loading

Codecov Report

damiannolan May 24, 2023

Choose a reason for hiding this comment

damiannolan left a comment • edited Loading

Choose a reason for hiding this comment

damiannolan May 25, 2023

Choose a reason for hiding this comment

DimitrisJim May 25, 2023

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

colin-axner commented May 4, 2023 •

edited

Loading

codecov-commenter commented May 15, 2023 •

edited

Loading

damiannolan left a comment •

edited

Loading