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.

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

fix: use consensus state and client state from lightclient 232 #670

Merged
merged 51 commits into from
Sep 6, 2023

Conversation

ibrizsabin
Copy link
Collaborator

@ibrizsabin ibrizsabin commented Aug 29, 2023

Description:

Commit Message

fix:  use consensus state and client state from lightclient 232

see the guidelines for commit messages.

Changelog Entry

version: <log entry>

Checklist:

  • I have performed a self-review of my own code
  • I have documented my code in accordance with the documentation guidelines
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have run the unit tests
  • I only have one commit (if not, squash them into one commit).
  • I have a descriptive commit message that adheres to the commit message guidelines

Please review the CONTRIBUTING.md file for detailed contributing guidelines.

@ibrizsabin ibrizsabin added Archway Archway Chain related task Audit Issue Issues discovered in security audit labels Aug 29, 2023
Base automatically changed from fix/security-review-180-183-185 to main August 29, 2023 08:02
@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2023

Codecov Report

Merging #670 (221b2e9) into main (dab4a1e) will decrease coverage by 0.11%.
Report is 3 commits behind head on main.
The diff coverage is 80.34%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #670      +/-   ##
============================================
- Coverage     68.95%   68.84%   -0.11%     
- Complexity      405      406       +1     
============================================
  Files           151      152       +1     
  Lines         13810    13863      +53     
  Branches        290      290              
============================================
+ Hits           9522     9544      +22     
- Misses         4136     4167      +31     
  Partials        152      152              
Flag Coverage Δ
java 80.92% <ø> (+0.08%) ⬆️
rust 66.81% <80.34%> (-0.14%) ⬇️

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

Files Changed Coverage Δ
contracts/cosmwasm-vm/cw-common/src/client_msg.rs 94.02% <ø> (ø)
contracts/cosmwasm-vm/cw-ibc-core/src/contract.rs 39.60% <0.00%> (-4.89%) ⬇️
contracts/cosmwasm-vm/cw-ibc-core/src/error.rs 34.14% <ø> (ø)
contracts/cosmwasm-vm/cw-ibc-core/src/lib.rs 42.85% <ø> (ø)
contracts/cosmwasm-vm/cw-ibc-core/src/state.rs 93.56% <ø> (-0.29%) ⬇️
...tracts/cosmwasm-vm/cw-ibc-core/src/storage_keys.rs 96.29% <ø> (-0.26%) ⬇️
...s/cosmwasm-vm/cw-icon-light-client/src/contract.rs 54.49% <0.00%> (-0.33%) ⬇️
...mwasm-vm/cw-icon-light-client/src/query_handler.rs 37.15% <0.00%> (-2.36%) ⬇️
...cosmwasm-vm/cw-ibc-core/src/ics02_client/client.rs 83.91% <64.70%> (-6.71%) ⬇️
...sm-vm/cw-ibc-core/src/light_client/light_client.rs 88.34% <80.76%> (+4.40%) ⬆️
... and 8 more

... and 9 files with indirect coverage changes

@ibrizsabin ibrizsabin added the bug Something isn't working label Aug 30, 2023
@ibrizsabin ibrizsabin changed the title Fix/security review 232 fix: use consensus state and client state from lightclient 232 Aug 30, 2023
@ibrizsabin ibrizsabin marked this pull request as ready for review August 30, 2023 11:47
@AntonAndell
Copy link
Collaborator

I am guessing the decrease in coverage comes from removal of code?
While in testing mode, might be good if you double check what parts are missing in the client logic and see if tests are needed.

AntonAndell
AntonAndell previously approved these changes Aug 30, 2023
Copy link
Collaborator

@AntonAndell AntonAndell left a comment

Choose a reason for hiding this comment

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

I think it looks fine, approved if the E2E passes

* fix: fix tests

* chore: pass build

* fix: validate packet expiry

* fix: add unit test for timeout

* chore: pass build

* fix: add test for packet timeout

* chore: pass build

* chore: remove cmnts

* fix: fix validate packet expired

* fix: timeout test

* chore: build pass

* fix: fix unit test

* chore: pass build

* fix: resolve conversation

* fix: fix test

* fix: timeout test

* chore: pass build

---------

Co-authored-by: sabinchitrakar <immortal.infidel@gmail.com>
ibrizsabin and others added 2 commits September 6, 2023 13:27
* fix: get client type from client state

* fix: rm log

* fix: remove reply from create client

* fix: update create client

* fix: remove sending root

* chore: build check

* fix: store client id on update

* fix: rename client

* chore: pass build

* fix: build script

* fix: fix build

---------

Co-authored-by: sabinchitrakar <immortal.infidel@gmail.com>
Co-authored-by: redlarva <91685111+redlarva@users.noreply.github.com>
Copy link
Collaborator

@redlarva redlarva left a comment

Choose a reason for hiding this comment

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

looks good and the end-to-end (e2e) run completed successfully.

@redlarva redlarva merged commit 1992b51 into main Sep 6, 2023
9 checks passed
@redlarva redlarva deleted the fix/security-review-232 branch September 6, 2023 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Archway Archway Chain related task Audit Issue Issues discovered in security audit bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants