-
Notifications
You must be signed in to change notification settings - Fork 586
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
imp: transfer total escrow follow ups #3558
Conversation
* add docs for total escrow feature * revert change * fix tag * Update docs/migrations/v7-to-v7_1.md --------- Co-authored-by: colin axnér <[email protected]>
* some more review comments * Rename pathAndEscrowAMount to pathAndEscrowAmount --------- Co-authored-by: DimitrisJim <[email protected]>
* 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 <[email protected]> --------- Co-authored-by: Colin Axnér <[email protected]> Co-authored-by: Damian Nolan <[email protected]>
# Conflicts: # modules/apps/transfer/keeper/migrations_test.go # modules/apps/transfer/keeper/relay_test.go
Codecov Report
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
|
* do not store 0 escrow amout * adapt success test * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Jim Fasarakis-Hilliard <[email protected]> * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Damian Nolan <[email protected]> * add comments --------- Co-authored-by: Jim Fasarakis-Hilliard <[email protected]> Co-authored-by: Damian Nolan <[email protected]>
…ry the endpoint in e2e (#3605)
e2e/testvalues/values.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more to add here after conflicts are resolved #3553 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could use a var block to declare all feature releases but I think having them separate is also nice and readable!
[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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just dropping a comment here to make sure this TODO is not forgotten
Co-authored-by: Jim Fasarakis-Hilliard <[email protected]> Co-authored-by: Damian Nolan <[email protected]>
* docs: add adr 011 for total escrow state entry * indentation * code formatting * Apply suggestions from code review Co-authored-by: Damian Nolan <[email protected]> * Apply suggestions from code review Co-authored-by: Charly <[email protected]> --------- Co-authored-by: Damian Nolan <[email protected]> Co-authored-by: Charly <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK, LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]> * transfer (total escrow): some more review comments (#3519) * some more review comments * Rename pathAndEscrowAMount to pathAndEscrowAmount --------- Co-authored-by: DimitrisJim <[email protected]> * 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 <[email protected]> --------- Co-authored-by: Colin Axnér <[email protected]> Co-authored-by: Damian Nolan <[email protected]> * 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 <[email protected]> * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Damian Nolan <[email protected]> * add comments --------- Co-authored-by: Jim Fasarakis-Hilliard <[email protected]> Co-authored-by: Damian Nolan <[email protected]> * 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 <[email protected]> Co-authored-by: Damian Nolan <[email protected]> * 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 <[email protected]> * Apply suggestions from code review Co-authored-by: Charly <[email protected]> --------- Co-authored-by: Damian Nolan <[email protected]> Co-authored-by: Charly <[email protected]> --------- Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: DimitrisJim <[email protected]> Co-authored-by: Damian Nolan <[email protected]> Co-authored-by: Charly <[email protected]> (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 <[email protected]> * transfer (total escrow): some more review comments (#3519) * some more review comments * Rename pathAndEscrowAMount to pathAndEscrowAmount --------- Co-authored-by: DimitrisJim <[email protected]> * 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 <[email protected]> --------- Co-authored-by: Colin Axnér <[email protected]> Co-authored-by: Damian Nolan <[email protected]> * 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 <[email protected]> * Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Damian Nolan <[email protected]> * add comments --------- Co-authored-by: Jim Fasarakis-Hilliard <[email protected]> Co-authored-by: Damian Nolan <[email protected]> * 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 <[email protected]> Co-authored-by: Damian Nolan <[email protected]> * 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 <[email protected]> * Apply suggestions from code review Co-authored-by: Charly <[email protected]> --------- Co-authored-by: Damian Nolan <[email protected]> Co-authored-by: Charly <[email protected]> --------- Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: DimitrisJim <[email protected]> Co-authored-by: Damian Nolan <[email protected]> Co-authored-by: Charly <[email protected]> (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 <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]>
Description
feature branch containing changes approved in smaller prs:
ref: #3368
Commit Message / Changelog Entry
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.
docs/
) or specification (x/<module>/spec/
).godoc
comments.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.