Cleanup mock context for ICS2 #305

adizere · 2020-10-12T20:29:45Z

Closes: #296

For contributor use:

Unit tests written
Added test to CI if applicable
Updated CHANGELOG_PENDING.md
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Updated relevant documentation (docs/) and code comments
Re-reviewed Files changed in the Github PR explorer

romac

👍

ancazamfir

Looks great, left a few comments.

modules/src/mock_context.rs

modules/src/ics02_client/handler.rs

modules/src/ics02_client/handler/update_client.rs

Other changes are required

codecov-io · 2020-10-14T14:09:09Z

Codecov Report

Merging #305 into master will increase coverage by 24.1%.
The diff coverage is 61.5%.

@@            Coverage Diff            @@
##           master    #305      +/-   ##
=========================================
+ Coverage    13.6%   37.7%   +24.1%     
=========================================
  Files          69     124      +55     
  Lines        3752    8013    +4261     
  Branches     1374    2778    +1404     
=========================================
+ Hits          513    3027    +2514     
- Misses       2618    4751    +2133     
+ Partials      621     235     -386

Impacted Files	Coverage Δ
modules/src/events.rs	`0.0% <0.0%> (ø)`
modules/src/ics02_client/events.rs	`0.0% <ø> (ø)`
modules/src/ics02_client/raw.rs	`0.0% <0.0%> (ø)`
modules/src/ics03_connection/context.rs	`0.0% <0.0%> (ø)`
modules/src/ics03_connection/error.rs	`16.6% <0.0%> (-16.7%)`	⬇️
modules/src/ics04_channel/error.rs	`75.0% <ø> (+50.0%)`	⬆️
modules/src/ics04_channel/packet.rs	`0.0% <0.0%> (ø)`
modules/src/ics07_tendermint/client_def.rs	`0.0% <0.0%> (ø)`
modules/src/ics07_tendermint/error.rs	`75.0% <ø> (+75.0%)`	⬆️
modules/src/ics18_relayer/error.rs	`0.0% <0.0%> (ø)`
... and 226 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8a7c65...53dec60. Read the comment docs.

ancazamfir

Looks good, a couple comments on dropping the ics2_...

modules/src/ics02_client/handler.rs

Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>

ancazamfir

Great!!!

ancazamfir · 2020-10-14T18:50:55Z

modules/src/mock_context.rs

+ pub fn with_client_parametrized(
+ self,
+ client_id: &ClientId,
+ client_state_height: Height,
+ client_type: Option<ClientType>,
+ consensus_state_height: Option<Height>,


Still not sure about this, seems that the client_state_height is a bit redundant, not clear what it means if the two heights are different. Maybe just pass an AnyHeader. But let's leave this for the next step where we support tendermint client and consensus states in the mock context and things will become more clear.

Will merge this after #309. Thanks Anca!

romac

LGTM

ancazamfir · 2020-10-15T14:59:41Z

modules/src/ics02_client/handler/create_client.rs

+ let ctx = MockContext::default().with_client_parametrized(
+  &client_id,
+ height,
+ Some(ClientType::Mock),
+  Some(height.increment()),
+ );
+ let height = Height::new(0, 30);


Note that this is does not result in the same context as before (where two consensus states existed). Another problem is that the context now has a client state with latest height smaller than the highest height of the consensus states, only one here but still an invariant that we don't currently have/check is broken. Maybe add this in a different PR. Until then just remove the increment() to pass the same height for client and consensus states.

ancazamfir · 2020-10-15T15:02:34Z

modules/src/mock_context.rs

+ // All the connections in the store.
+ connections: HashMap<ConnectionId, ConnectionEnd>,
+


can we move these back down again? :)

ICS2 create_client handler adapted to global mock context.

41aa2c2

adizere self-assigned this Oct 12, 2020

adizere changed the title ~~ICS2 create_client handler adapted to global mock context~~ Cleanup mock context for ICS2 Oct 13, 2020

adizere added 3 commits October 13, 2020 17:12

Cleaning update_client.rs

d10f5a4

Merge branch 'master' into adi/296_ics2_cleanup

19b500b

Testing w/ dispatch() instead of process()

cb98847

adizere marked this pull request as ready for review October 13, 2020 18:50

adizere requested review from ancazamfir and romac as code owners October 13, 2020 18:50

adizere marked this pull request as draft October 13, 2020 19:04

adizere marked this pull request as ready for review October 13, 2020 19:04

More asserts, avoided explicit type annotations.

d012f6c

romac previously approved these changes Oct 14, 2020

View reviewed changes

ancazamfir reviewed Oct 14, 2020

View reviewed changes

modules/src/mock_context.rs Show resolved Hide resolved

modules/src/mock_context.rs Outdated Show resolved Hide resolved

modules/src/ics02_client/handler.rs Outdated Show resolved Hide resolved

modules/src/ics02_client/handler/update_client.rs Outdated Show resolved Hide resolved

adizere added 2 commits October 14, 2020 15:52

Wrapper around parametrized & remark about invalid context.

a6d3d2e

Switched from 'result' to 'output'

c1f8a4f

adizere added 2 commits October 14, 2020 19:09

Fixes after Anca's detailed comments & chat

fbbde8e

Reverted the renaming of ClientEvent enum members.

ba81443

ancazamfir reviewed Oct 14, 2020

View reviewed changes

modules/src/ics02_client/handler.rs Outdated Show resolved Hide resolved

modules/src/ics02_client/handler.rs Outdated Show resolved Hide resolved

modules/src/ics02_client/handler.rs Outdated Show resolved Hide resolved

Apply Anca's suggestions to simplify imports in ICS2 handler

53dec60

Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>

ancazamfir approved these changes Oct 14, 2020

View reviewed changes

adizere added 2 commits October 15, 2020 15:37

Merge branch 'master' into adi/296_ics2_cleanup

152a8b8

Post-merge re-fixes.

831b378

romac approved these changes Oct 15, 2020

View reviewed changes

adizere merged commit ec494b1 into master Oct 15, 2020

adizere deleted the adi/296_ics2_cleanup branch October 15, 2020 14:52

ancazamfir reviewed Oct 15, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup mock context for ICS2 #305

Cleanup mock context for ICS2 #305

adizere commented Oct 12, 2020

romac left a comment

ancazamfir left a comment

codecov-io commented Oct 14, 2020 •

edited

Loading

ancazamfir left a comment

ancazamfir left a comment

ancazamfir Oct 14, 2020

adizere Oct 15, 2020

romac left a comment

ancazamfir Oct 15, 2020

adizere Oct 16, 2020

ancazamfir Oct 15, 2020

adizere Oct 16, 2020

		// All the connections in the store.
		connections: HashMap<ConnectionId, ConnectionEnd>,

Cleanup mock context for ICS2 #305

Cleanup mock context for ICS2 #305

Conversation

adizere commented Oct 12, 2020

romac left a comment

Choose a reason for hiding this comment

ancazamfir left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 14, 2020 • edited Loading

Codecov Report

ancazamfir left a comment

Choose a reason for hiding this comment

ancazamfir left a comment

Choose a reason for hiding this comment

ancazamfir Oct 14, 2020

Choose a reason for hiding this comment

adizere Oct 15, 2020

Choose a reason for hiding this comment

romac left a comment

Choose a reason for hiding this comment

ancazamfir Oct 15, 2020

Choose a reason for hiding this comment

adizere Oct 16, 2020

Choose a reason for hiding this comment

ancazamfir Oct 15, 2020

Choose a reason for hiding this comment

adizere Oct 16, 2020

Choose a reason for hiding this comment

codecov-io commented Oct 14, 2020 •

edited

Loading