refactor: memo parsing logic in ibc_packet_handlers.go

[aljo242]

1. Summary

refactor duplicated logic in x/interchainstaking/keeper/ibc_packet_handlers.go into a types package.

2.Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)

3. Implementation details

• Refactor duplicated code into types package
• add tests
• add fuzz tests

4. How to test/use

To run the fuzz test:

go test -fuzz=FuzzParseMsgMemo github.com/ingenuity-build/quicksilver/x/interchainstaking/types -fuzztime=5m 

5. Checklist

• Does the Readme need to be updated?

6. Limitations (optional)

7. Future Work (optional)

[codecov-commenter]

Codecov Report

Merging #321 (69ef6a9) into main (f5897f4) will increase coverage by 0.08%.
The diff coverage is 76.47%.

@@            Coverage Diff             @@
##             main     #321      +/-   ##
==========================================
+ Coverage   56.22%   56.31%   +0.08%     
==========================================
  Files         153      154       +1     
  Lines       12057    12051       -6     
==========================================
+ Hits         6779     6786       +7     
+ Misses       4805     4795      -10     
+ Partials      473      470       -3     

Flag	Coverage Δ
unittests	56.31% <76.47%> (+0.08%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
x/interchainstaking/keeper/ibc_packet_handlers.go	31.37% <42.85%> (+0.22%)	⬆️
x/interchainstaking/types/ibc_packet.go	100.00% <100.00%> (ø)

[joe-bowman]

Looks good overall, but as discussed there seems to be no difference between the fuzz test and the unit test here in terms of coverage. As fuzz tests are not currently run on every push, and the unit test is not resource intensive, should be just keep that?

[ajansari95]

Looks good overall, but as discussed there seems to be no difference between the fuzz test and the unit test here in terms of coverage. As fuzz tests are not currently run on every push, and the unit test is not resource intensive, should be just keep that?

i think we can add a fuzz test target in make file and not include in coverage?

[aljo242]

@ajansari95 @joe-bowman makes most sense to remove the fuzz test in this case because as @joe-bowman mentioned, its not giving us much more than the unit test which we are running all of the time.

[joe-bowman]

lgtm

[ajansari95]

LGTM