transfer (total escrow): add invariant for total escrow #3506

crodriguezvega · 2023-04-22T21:06:10Z

Description

Commit Message / Changelog Entry

N/A

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.

codecov-commenter · 2023-04-22T21:12:43Z

Codecov Report

Merging #3506 (87240a3) into feat/total-escrow-state-entry (fa9418f) will increase coverage by 0.01%.
The diff coverage is 87.09%.

Additional details and impacted files

@@                        Coverage Diff                        @@
##           feat/total-escrow-state-entry    #3506      +/-   ##
=================================================================
+ Coverage                          78.78%   78.79%   +0.01%     
=================================================================
  Files                                182      183       +1     
  Lines                              12687    12715      +28     
=================================================================
+ Hits                                9995    10019      +24     
- Misses                              2261     2265       +4     
  Partials                             431      431

Impacted Files	Coverage Δ
modules/apps/transfer/keeper/invariants.go	`85.71% <85.71%> (ø)`
modules/apps/transfer/keeper/keeper.go	`88.88% <100.00%> (ø)`
modules/apps/transfer/module.go	`54.54% <100.00%> (ø)`

modules/apps/transfer/keeper/invariants.go

colin-axner

I applied my own suggestions, would appreciate 2 more approvals

colin-axner · 2023-04-27T09:53:54Z

modules/apps/transfer/keeper/invariants.go

+			return sdk.FormatInvariant(
+				types.ModuleName,
+				"total escrow per denom invariance",
+				fmt.Sprintf("found denom(s) with total escrow amount lower than expected:\nactual total escrowed: %s\nexpected total escrowed: %s", actualTotalEscrowed, expectedTotalEscrowed)), true


The full error msg isn't as nice, as it'll contain all denoms rather than just the ones of out sync, but I find this code a lot simpler and easier to verify correctness. This should be a very rare invariant to break (hopefully it never breaks)

colin-axner · 2023-04-27T09:54:16Z

modules/apps/transfer/keeper/invariants.go

+}
+
+// AllInvariants runs all invariants of the transfer module.
+func AllInvariants(k *Keeper) sdk.Invariant {


I wonder if we should add a call to this in the tests?

damiannolan · 2023-05-02T10:48:28Z

Looks like some unit tests are failing due to the new invariant added(?)

modules/apps/transfer/keeper/keeper.go

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

damiannolan

Nice work! Looks good to me! 🚀

damiannolan · 2023-05-03T15:59:14Z

modules/apps/transfer/keeper/invariants.go

+
+// RegisterInvariants registers all transfer invariants
+func RegisterInvariants(ir sdk.InvariantRegistry, k *Keeper) {
+	ir.RegisterRoute(types.ModuleName, "total-escrow-per-denom",


Assuming this string is arbitrary and doesn't hold too much significance?

doesn't seem to be treated as very significant in the sdk equivalent impl.

colin-axner · 2023-05-04T09:28:11Z

merging!

* 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>

* 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>

add invariant for total escrow

fc6db2b

crodriguezvega requested review from colin-axner, AdityaSripal and damiannolan as code owners April 22, 2023 21:06

crodriguezvega mentioned this pull request Apr 22, 2023

Follow up items for #3019 #3368

Closed

7 tasks

crodriguezvega changed the title ~~transfer: add invariant for total escrow~~ transfer (total escrow): add invariant for total escrow Apr 23, 2023

DimitrisJim reviewed Apr 24, 2023

View reviewed changes

modules/apps/transfer/keeper/invariants.go Outdated Show resolved Hide resolved

modules/apps/transfer/keeper/invariants.go Outdated Show resolved Hide resolved

address review comment

4361d2b

colin-axner reviewed Apr 27, 2023

View reviewed changes

modules/apps/transfer/keeper/invariants.go Outdated Show resolved Hide resolved

modules/apps/transfer/keeper/invariants.go Outdated Show resolved Hide resolved

refactor: simplify logic for asserting invariant

43a06e3

colin-axner approved these changes Apr 27, 2023

View reviewed changes

colin-axner requested review from chatton and charleenfei April 27, 2023 09:55

fix: use safeAdd instead of append

d3d3bda

damiannolan reviewed May 3, 2023

View reviewed changes

modules/apps/transfer/keeper/keeper.go Outdated Show resolved Hide resolved

Update modules/apps/transfer/keeper/keeper.go

87240a3

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

damiannolan approved these changes May 3, 2023

View reviewed changes

colin-axner merged commit f72de1f into feat/total-escrow-state-entry May 4, 2023

colin-axner deleted the carlos/add-invariant-total-escrow branch May 4, 2023 09:28

colin-axner mentioned this pull request May 4, 2023

imp: transfer total escrow follow ups #3558

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transfer (total escrow): add invariant for total escrow #3506

transfer (total escrow): add invariant for total escrow #3506

crodriguezvega commented Apr 22, 2023

codecov-commenter commented Apr 22, 2023 •

edited

Loading

colin-axner left a comment

colin-axner Apr 27, 2023

colin-axner Apr 27, 2023

damiannolan commented May 2, 2023

damiannolan left a comment

damiannolan May 3, 2023

DimitrisJim May 3, 2023

colin-axner commented May 4, 2023

transfer (total escrow): add invariant for total escrow #3506

transfer (total escrow): add invariant for total escrow #3506

Conversation

crodriguezvega commented Apr 22, 2023

Description

Commit Message / Changelog Entry

codecov-commenter commented Apr 22, 2023 • edited Loading

Codecov Report

colin-axner left a comment

Choose a reason for hiding this comment

colin-axner Apr 27, 2023

Choose a reason for hiding this comment

colin-axner Apr 27, 2023

Choose a reason for hiding this comment

damiannolan commented May 2, 2023

damiannolan left a comment

Choose a reason for hiding this comment

damiannolan May 3, 2023

Choose a reason for hiding this comment

DimitrisJim May 3, 2023

Choose a reason for hiding this comment

colin-axner commented May 4, 2023

codecov-commenter commented Apr 22, 2023 •

edited

Loading