Re-enable tests in `ics02_client` and `foreign_client` #535

adizere · 2021-01-19T11:00:28Z

Closes: #469

This PR re-enables the tests in the ics02_client and foreign_client modules.

The ibc::handler::Event type was removed. Handlers now produce IBCEvents.

For contributor use:

Updated the Unreleased section of CHANGELOG.md with the issue.
If applicable: Unit tests written, added test to CI.
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.

codecov-io · 2021-01-19T13:40:07Z

Codecov Report

Merging #535 (03c2469) into master (b1b37f5) will increase coverage by 31.9%.
The diff coverage is 67.8%.

@@            Coverage Diff            @@
##           master    #535      +/-   ##
=========================================
+ Coverage    13.6%   45.5%   +31.9%     
=========================================
  Files          69     134      +65     
  Lines        3752    8640    +4888     
  Branches     1374       0    -1374     
=========================================
+ Hits          513    3938    +3425     
- Misses       2618    4702    +2084     
+ Partials      621       0     -621

Impacted Files	Coverage Δ
...application/ics20_fungible_token_transfer/error.rs	`0.0% <0.0%> (ø)`
...pplication/ics20_fungible_token_transfer/events.rs	`0.0% <ø> (ø)`
...ion/ics20_fungible_token_transfer/msgs/transfer.rs	`0.0% <0.0%> (ø)`
modules/src/events.rs	`0.0% <0.0%> (ø)`
modules/src/ics02_client/error.rs	`100.0% <ø> (ø)`
modules/src/ics02_client/raw.rs	`0.0% <0.0%> (ø)`
modules/src/ics03_connection/error.rs	`100.0% <ø> (+66.6%)`	⬆️
modules/src/ics03_connection/msgs/conn_open_try.rs	`86.7% <ø> (ø)`
modules/src/ics03_connection/version.rs	`97.6% <ø> (ø)`
modules/src/ics04_channel/channel.rs	`71.2% <ø> (+56.1%)`	⬆️
... and 245 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 a1c393f...4ffe9bb. Read the comment docs.

…c-rs into adi/469_tests

vitorenesduarte · 2021-01-22T14:08:00Z

modules/src/ics02_client/context.rs

                    res.client_state.latest_height(),
                    res.consensus_state,
                )?;
-                Ok(vec![ClientEvent::ClientCreated(client_id)])


Events were being created both here and at the handlers. Now they're only created at the handlers.

This looks better & cleaner!

Just to note that In future (or in other handlers) we may find it useful to create events both from the process method of the handlers, as well as from the ClientKeeper. Then we would collect both of these vectors of events into a single one in ICS26::dispatch. But that's not for now..

vitorenesduarte · 2021-01-22T14:13:11Z

modules/src/ics02_client/events.rs

-        .collect()
+const CONSENSUS_HEIGHT_ATTRIBUTE_KEY: &str = "consensus_height";
+
+pub fn try_from_tx(event: &tendermint::abci::Event) -> Option<IBCEvent> {


This PR touches the translation from tendermint::abci::Event to IBCEvent (here and in other modules) but didn't have to. This was done while generalizing the translation to not be specific to tendermint::abci::Event. This turned out to not be necessary after removing the ibc::handler::Event. Nevertheless, in the process of generalising this, some improvements were made and we may want to keep them.

vitorenesduarte · 2021-01-22T14:27:22Z

modules/src/ics03_connection/handler/conn_open_try.rs

@@ -87,7 +88,13 @@ pub(crate) fn process(
        connection_end: new_connection_end,
    };

-    output.emit(ConnOpenTry(result.clone()));
+    // TODO: move connection id decision (`next_connection_id` method) in ClientReader


This TODO was moved from modules/src/ics03_connection/handler.rs

adizere

Looking good, left a few ideas.

adizere · 2021-01-25T08:40:27Z

modules/src/ics02_client/context.rs

                    res.client_state.latest_height(),
                    res.consensus_state,
                )?;
-                Ok(vec![ClientEvent::ClientCreated(client_id)])


This looks better & cleaner!

Just to note that In future (or in other handlers) we may find it useful to create events both from the process method of the handlers, as well as from the ClientKeeper. Then we would collect both of these vectors of events into a single one in ICS26::dispatch. But that's not for now..

adizere · 2021-01-25T08:43:12Z

modules/src/handler.rs

-}
-
-#[derive(Clone, Debug, PartialEq, Eq)]
-pub struct Event {


If you wish, I suggest once we're done with this we do a follow-up PR to update ADR003 events section, making it clear that handlers are using the strongly-typed events, not these loosely-typed events.

Can we open an issue for that? It's probably better if someone familiar with that ADR does the change.

adizere · 2021-01-25T08:52:38Z

modules/src/ics02_client/handler/create_client.rs

+        Kind::ClientIdentifierConstructor(msg.client_state().client_type(), id_counter).context(e)
+    })?;
+
+    output.log("success: generated new client identifier");


Just a heads-up: It may be a prerequisite for doing the MBT work that we use a more principled approach to emit these log lines. So maybe have a centralized place in the modules where we enumerate all the kinds of log lines; these could also be loosely coupled with IBCEvents (since they appear in tandem).

modules/src/ics04_channel/msgs.rs

modules/src/ics26_routing/handler.rs

relayer/src/foreign_client.rs

vitorenesduarte · 2021-01-25T11:28:04Z

Thanks for the review @adizere. I've addressed your suggestions.

…ms#535) * ClientId constructor; re-enabled ClientCreate tests * Improved ics18; Fixed cfg cargo annotation. * Prep for fixing ForeignClient tests. * Enable tests in the foreign_client module * Reuse Height parsing * More idiomatic methods in ibc::mock::context::MockContext * Improve assert in build_create_client_and_send * Remove ibc::handler::Event * Fix extract_connection_id * Check IBCEvent produced in create_client * Check IBCEvent produced in test_tm_create_client_ok * update CHANGELOG * relayer: improve tests in foreign_client Co-authored-by: Vitor Enes <vitorenesduarte@gmail.com>

adizere added 3 commits January 19, 2021 11:59

ClientId constructor; re-enabled ClientCreate tests

ec00b02

Improved ics18; Fixed cfg cargo annotation.

6518cb8

Merge branch 'master' into adi/469_tests

14c5cac

adizere and others added 3 commits January 19, 2021 17:07

Prep for fixing ForeignClient tests.

8ce3bb6

Merge branch 'master' into adi/469_tests

f0227bf

Enable tests in the foreign_client module

85e7e46

vitorenesduarte force-pushed the adi/469_tests branch from d60a38f to 85e7e46 Compare January 21, 2021 18:59

vitorenesduarte added 18 commits January 21, 2021 19:09

Refer to const event types in tests

9966edf

Reuse Height parsing

5c37cc5

Add comment

9355d4f

fmt

4092026

Make clippy happy

ba4a39d

Make clippy happy

e0c7383

add TODOs

9f6f639

add TODO

6658b10

More idiomatic methods in ibc::mock::context::MockContext

90b64e4

fix TODO

59f2036

Only create events in handlers

25be8ae

Improve assert in build_create_client_and_send

0741e75

Remove ibc::handler::Event

b991020

Make clippy happy

6ada439

Merge branch 'adi/469_tests' of https://github.com/informalsystems/ib…

e338044

…c-rs into adi/469_tests

Fix extract_connection_id

da8c37a

Merge branch 'master' into adi/469_tests

03c2469

address some TODOs

4ffe9bb

vitorenesduarte marked this pull request as ready for review January 22, 2021 14:06

vitorenesduarte requested review from ancazamfir and romac as code owners January 22, 2021 14:06

vitorenesduarte reviewed Jan 22, 2021

View reviewed changes

vitorenesduarte added 2 commits January 22, 2021 14:21

Check IBCEvent produced in create_client

935742f

Check IBCEvent produced in test_tm_create_client_ok

1db5815

vitorenesduarte reviewed Jan 22, 2021

View reviewed changes

vitorenesduarte changed the title ~~Testing & cleanup: follow-up to migration work~~ Re-enable tests in ics02_client and foreign_client Jan 22, 2021

adizere commented Jan 25, 2021

View reviewed changes

vitorenesduarte added 3 commits January 25, 2021 12:09

update CHANGELOG

84885c7

relayer: improve tests in foreign_client

e0db63c

fmt

c5f2af9

vitorenesduarte approved these changes Jan 25, 2021

View reviewed changes

adizere mentioned this pull request Jan 25, 2021

ADR 003 update to reflect recent changes around Events #548

Closed

5 tasks

vitorenesduarte merged commit d45fcee into master Jan 25, 2021

vitorenesduarte deleted the adi/469_tests branch February 13, 2021 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-enable tests in `ics02_client` and `foreign_client` #535

Re-enable tests in `ics02_client` and `foreign_client` #535

adizere commented Jan 19, 2021 •

edited

Loading

codecov-io commented Jan 19, 2021 •

edited

Loading

vitorenesduarte Jan 22, 2021

adizere Jan 25, 2021

vitorenesduarte Jan 22, 2021

vitorenesduarte Jan 22, 2021

adizere left a comment

adizere Jan 25, 2021

adizere Jan 25, 2021

vitorenesduarte Jan 25, 2021

adizere Jan 25, 2021

vitorenesduarte commented Jan 25, 2021

Re-enable tests in ics02_client and foreign_client #535

Re-enable tests in ics02_client and foreign_client #535

Conversation

adizere commented Jan 19, 2021 • edited Loading

codecov-io commented Jan 19, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adizere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitorenesduarte commented Jan 25, 2021

Re-enable tests in `ics02_client` and `foreign_client` #535

Re-enable tests in `ics02_client` and `foreign_client` #535

adizere commented Jan 19, 2021 •

edited

Loading

codecov-io commented Jan 19, 2021 •

edited

Loading