fix: support chain identifiers without `{chain_name}-{revision_number}` pattern #941

rnbguy · 2023-11-01T14:57:19Z

Closes: #940
Closes: #943

Description

ibc-go allows a hardcoded revision number for a chain-id without revision number.

Currently, ibc-rs requires all chain-ids to have a revision number. This PR allows ChainId struct to accept chain ID strings without revision numbers.

This PR also adds an optimized implementation for validate_length_prefix.

PR author checklist:

Added changelog entry, using unclog.
Added tests.
Linked to GitHub issue.
Updated code comments and documentation (e.g., docs/).
Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

codecov · 2023-11-01T15:03:05Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (553bc79) 67.65% compared to head (091f291) 67.84%.
Report is 2 commits behind head on main.

❗ Current head 091f291 differs from pull request most recent head 5782666. Consider uploading reports for the commit 5782666 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #941      +/-   ##
==========================================
+ Coverage   67.65%   67.84%   +0.18%     
==========================================
  Files         130      130              
  Lines       16415    16431      +16     
==========================================
+ Hits        11106    11147      +41     
+ Misses       5309     5284      -25

Files	Coverage Δ
...s/ibc/src/clients/ics07_tendermint/client_state.rs	`74.31% <100.00%> (ø)`
...ibc/src/core/ics02_client/handler/update_client.rs	`96.33% <ø> (ø)`
...src/core/ics03_connection/handler/conn_open_ack.rs	`94.56% <100.00%> (ø)`
...src/core/ics03_connection/handler/conn_open_try.rs	`91.26% <100.00%> (ø)`
crates/ibc/src/mock/context.rs	`72.81% <100.00%> (ø)`
crates/ibc/src/mock/ics18_relayer/context.rs	`65.00% <ø> (ø)`
crates/ibc/src/core/ics24_host/identifier.rs	`68.10% <98.30%> (+1.86%)`	⬆️
...tes/ibc/src/core/ics24_host/identifier/validate.rs	`96.66% <50.00%> (-3.34%)`	⬇️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Farhad-Shabani

Thanks @rnbguy for taking care of this 🙏🏻
Since the ChainId's data structure has changed here and other changes are related, let's go over that first:

crates/ibc/src/core/ics24_host/identifier.rs

.changelog/unreleased/bug-fixes/940-optional-revision-number-in-chain-id.md

…number

codecov · 2023-11-03T18:59:23Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (553bc79) 67.65% compared to head (94ede00) 67.89%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #941      +/-   ##
==========================================
+ Coverage   67.65%   67.89%   +0.23%     
==========================================
  Files         130      130              
  Lines       16415    16449      +34     
==========================================
+ Hits        11106    11168      +62     
+ Misses       5309     5281      -28

Files	Coverage Δ
...s/ibc/src/clients/ics07_tendermint/client_state.rs	`74.31% <100.00%> (ø)`
...ibc/src/core/ics02_client/handler/update_client.rs	`96.33% <ø> (ø)`
...src/core/ics03_connection/handler/conn_open_ack.rs	`94.56% <100.00%> (ø)`
...src/core/ics03_connection/handler/conn_open_try.rs	`91.26% <100.00%> (ø)`
crates/ibc/src/core/ics24_host/identifier.rs	`68.93% <100.00%> (+2.70%)`	⬆️
crates/ibc/src/mock/context.rs	`72.81% <100.00%> (ø)`
crates/ibc/src/mock/ics18_relayer/context.rs	`65.00% <ø> (ø)`
...tes/ibc/src/core/ics24_host/identifier/validate.rs	`99.04% <96.29%> (-0.96%)`	⬇️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Farhad-Shabani

Thanks @rnbguy for digging into this issue! Great job!
Just a few nits, then we are good to go!

.changelog/unreleased/bug-fixes/940-optional-revision-number-in-chain-id.md

crates/ibc/src/core/ics24_host/identifier.rs

Co-authored-by: Farhad Shabani <Farhad.Shabani@gmail.com> Signed-off-by: Rano | Ranadeep <ranadip.bswas@gmail.com>

crates/ibc/src/core/ics24_host/identifier.rs

crates/ibc/src/core/ics24_host/identifier/validate.rs

Co-authored-by: Farhad Shabani <Farhad.Shabani@gmail.com> Signed-off-by: Rano | Ranadeep <ranadip.bswas@gmail.com>

Farhad-Shabani

🎉

…}` pattern (#941) * optional revision number for chain id * add additional methods * update tests * add changelog entry * new test for codecov * revert optional u64 * rename method * update tests * visualize actual testcases * revert method rename * update changelog entry * refactor ChainId::new * rm ChainId::has_revision_number method * refactor ChainId::set_revision_number to ChainId::increment_revision_number * rm old set unset revision_number test * add tests for ChainId::increment_revision_number * add length validation tests * refactor source code for ChainId::new * fix doc test * fix chain identifier length validation * update tests * fix typo * reword changelog entry * reword doc string * fix ChainId::validate_length * add ChainId::validate_length in tests * replace ID with identifier * use validate_prefix_length over validate_identifier_length * optimize validate_prefix_length * rename changelog entry * fix incorrect doc comment Co-authored-by: Farhad Shabani <Farhad.Shabani@gmail.com> Signed-off-by: Rano | Ranadeep <ranadip.bswas@gmail.com> * use u64::checked_add * update doc comment * update doc tests * simplify validate_prefix_length * tests for validate_prefix_length * fix tests * add changelog entry * import test_log::test * fix rstest test attribute * cargo fmt * fix typo Co-authored-by: Farhad Shabani <Farhad.Shabani@gmail.com> Signed-off-by: Rano | Ranadeep <ranadip.bswas@gmail.com> --------- Signed-off-by: Rano | Ranadeep <ranadip.bswas@gmail.com> Co-authored-by: Farhad Shabani <Farhad.Shabani@gmail.com>

rnbguy added 3 commits November 1, 2023 10:51

optional revision number for chain id

c8d3584

add additional methods

f93ea96

update tests

6a6fc4f

rnbguy requested a review from Farhad-Shabani November 1, 2023 14:57

add changelog entry

674501f

new test for codecov

4296b0e

Farhad-Shabani requested changes Nov 2, 2023

View reviewed changes

crates/ibc/src/core/ics24_host/identifier.rs Outdated Show resolved Hide resolved

rnbguy added 4 commits November 2, 2023 12:09

revert optional u64

03301c2

rename method

a068d57

update tests

f0c8c21

visualize actual testcases

840fabd

rnbguy changed the title ~~fix: use optional revision number in ChainId~~ fix: handle ChainIds without revision numbers Nov 2, 2023

rnbguy added 2 commits November 2, 2023 12:22

revert method rename

60cf0cf

update changelog entry

6c89049

Farhad-Shabani requested changes Nov 2, 2023

View reviewed changes

rnbguy self-assigned this Nov 3, 2023

rnbguy added 12 commits November 3, 2023 13:56

refactor ChainId::new

a3b6fd9

rm ChainId::has_revision_number method

b8534b7

refactor ChainId::set_revision_number to ChainId::increment_revision_…

51d5770

…number

rm old set unset revision_number test

63ac38f

add tests for ChainId::increment_revision_number

257152e

add length validation tests

ad57856

refactor source code for ChainId::new

ebe39ea

fix doc test

20302f0

fix chain identifier length validation

6cc3d28

update tests

23ca22e

fix typo

091f291

reword changelog entry

5782666

rnbguy added 3 commits November 3, 2023 15:30

reword doc string

22f5b8f

fix ChainId::validate_length

02fb312

add ChainId::validate_length in tests

a19ac45

rnbguy force-pushed the rano/fix/chain-id-rev-number branch from fcf02fd to a19ac45 Compare November 3, 2023 19:42

Farhad-Shabani reviewed Nov 3, 2023

View reviewed changes

.changelog/unreleased/bug-fixes/940-optional-revision-number-in-chain-id.md Outdated Show resolved Hide resolved

crates/ibc/src/core/ics24_host/identifier.rs Outdated Show resolved Hide resolved

Farhad-Shabani mentioned this pull request Nov 3, 2023

Tracks ibc-rs PR #939, #941, #949 informalsystems/basecoin-rs#143

Merged

1 task

rnbguy added 3 commits November 3, 2023 17:16

replace ID with identifier

e8375b0

use validate_prefix_length over validate_identifier_length

a4648d7

optimize validate_prefix_length

69301f8

rnbguy changed the title ~~fix: handle ChainIds without revision numbers~~ fix: support chain identifiers without {chain_name}-{revision_number} pattern Nov 3, 2023

rnbguy and others added 2 commits November 3, 2023 18:08

rename changelog entry

5302611

fix incorrect doc comment

1ee65f2

Co-authored-by: Farhad Shabani <Farhad.Shabani@gmail.com> Signed-off-by: Rano | Ranadeep <ranadip.bswas@gmail.com>

Farhad-Shabani reviewed Nov 3, 2023

View reviewed changes

crates/ibc/src/core/ics24_host/identifier.rs Outdated Show resolved Hide resolved

Farhad-Shabani mentioned this pull request Nov 4, 2023

feat: optimize IdentifierError variants #942

Merged

7 tasks

rnbguy added 7 commits November 4, 2023 10:49

use u64::checked_add

b53b5bd

update doc comment

784633b

update doc tests

c7c6619

simplify validate_prefix_length

a850f06

tests for validate_prefix_length

6c7ae89

fix tests

8281ce8

add changelog entry

198b991

Farhad-Shabani reviewed Nov 6, 2023

View reviewed changes

crates/ibc/src/core/ics24_host/identifier/validate.rs Outdated Show resolved Hide resolved

rnbguy and others added 4 commits November 6, 2023 13:34

import test_log::test

c0577b7

fix rstest test attribute

ae09ee2

cargo fmt

0e41b74

fix typo

94ede00

Co-authored-by: Farhad Shabani <Farhad.Shabani@gmail.com> Signed-off-by: Rano | Ranadeep <ranadip.bswas@gmail.com>

Farhad-Shabani approved these changes Nov 6, 2023

View reviewed changes

Farhad-Shabani merged commit 1ec4b8c into main Nov 6, 2023
13 checks passed

Farhad-Shabani deleted the rano/fix/chain-id-rev-number branch November 6, 2023 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: support chain identifiers without `{chain_name}-{revision_number}` pattern #941

fix: support chain identifiers without `{chain_name}-{revision_number}` pattern #941

rnbguy commented Nov 1, 2023 •

edited

Loading

codecov bot commented Nov 1, 2023 •

edited

Loading

Farhad-Shabani left a comment

codecov bot commented Nov 3, 2023 •

edited

Loading

Farhad-Shabani left a comment

Farhad-Shabani left a comment

fix: support chain identifiers without {chain_name}-{revision_number} pattern #941

fix: support chain identifiers without {chain_name}-{revision_number} pattern #941

Conversation

rnbguy commented Nov 1, 2023 • edited Loading

Description

PR author checklist:

Reviewer checklist:

codecov bot commented Nov 1, 2023 • edited Loading

Codecov Report

Farhad-Shabani left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 3, 2023 • edited Loading

Codecov Report

Farhad-Shabani left a comment

Choose a reason for hiding this comment

Farhad-Shabani left a comment

Choose a reason for hiding this comment

fix: support chain identifiers without `{chain_name}-{revision_number}` pattern #941

fix: support chain identifiers without `{chain_name}-{revision_number}` pattern #941

rnbguy commented Nov 1, 2023 •

edited

Loading

codecov bot commented Nov 1, 2023 •

edited

Loading

codecov bot commented Nov 3, 2023 •

edited

Loading