Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

ics26_routing: Derive Hash for ModuleId #179

Merged
merged 3 commits into from
Oct 17, 2022
Merged

ics26_routing: Derive Hash for ModuleId #179

merged 3 commits into from
Oct 17, 2022

Conversation

kevinji
Copy link
Contributor

@kevinji kevinji commented Oct 13, 2022 •
edited
Loading

Signed-off-by: Kevin Ji 1146876+kevinji@users.noreply.github.com

Description

Derive Hash for ModuleId so it can be stored in a HashMap.

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).

@kevinji
ics26_routing: Derive Hash for ModuleId
d7a437b 
Signed-off-by: Kevin Ji <1146876+kevinji@users.noreply.github.com>
@DaviRain-Su
Copy link
Contributor

You can use BtreeMap, So We don't need this change.

@hu55a1n1
Copy link
Member

Thanks for the PR @kevinji!
A HashMap uses a randomly seeded hashing algorithm that results in a random iteration order and could introduce non-determinism in on-chain code. Please use BTreeMap as @DaviRain-Su suggested.
Please feel free to reopen if you have other needs that require the Hash impl. 🙏

@hu55a1n1 hu55a1n1 closed this Oct 14, 2022
@kevinji
Copy link
Contributor Author

kevinji commented Oct 14, 2022

Hey I had two questions:

  • I'm writing some code for Solana where rand is not enabled, so I believe HashMap is deterministic. Is this use case not supported?
  • I'm also making ModuleId consistent with PortId and ChannelId which do derive Hash. Is there a reason why ModuleId shouldn't work the same way?

@hu55a1n1
Copy link
Member

Thanks for providing this additional context. I think this is a valid use case.

@hu55a1n1 hu55a1n1 reopened this Oct 14, 2022
hu55a1n1
hu55a1n1 reviewed Oct 14, 2022
View reviewed changes
Copy link
Member

@hu55a1n1 hu55a1n1 left a comment

Choose a reason for hiding this comment

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

We're missing a changelog entry. Would you like to add one? Otherwise please feel free to delegate to us and we'll add it. See here for details on how to add a changelog using unclog -> https://github.com/cosmos/ibc-rs/blob/main/CONTRIBUTING.md#changelog.

@kevinji
Copy link
Contributor Author

kevinji commented Oct 14, 2022

Just added a changelog entry; let me know if it needs any changes.

hu55a1n1
hu55a1n1 approved these changes Oct 17, 2022
View reviewed changes
Copy link
Member

@hu55a1n1 hu55a1n1 left a comment

Choose a reason for hiding this comment

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

👌

@hu55a1n1 hu55a1n1 merged commit 8bd8235 into informalsystems:main Oct 17, 2022
@kevinji kevinji deleted the patch-1 branch October 17, 2022 16:32
plafer pushed a commit that referenced this pull request Oct 19, 2022
@kevinji @hu55a1n1 @plafer
ics26_routing: Derive Hash for ModuleId (#179)
28ff2a4 
* ics26_routing: Derive Hash for ModuleId

Signed-off-by: Kevin Ji <1146876+kevinji@users.noreply.github.com>

* Add changelog for #179

Signed-off-by: Kevin Ji <1146876+kevinji@users.noreply.github.com>
Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>
plafer added a commit that referenced this pull request Oct 24, 2022
@plafer @kevinji @hu55a1n1
Remove crossing hellos from Channel (#181)
1feb771 
* variable naming in ics26

* Rework ics26 channel handler

* cleanup Ics4ChannelMsg handler

* channel callbacks now return ModuleExtras

* channel_dispatch now returns a `MsgReceipt`

* remove crossing hellos logic from chan_open_try

Also removes tests that tested that logic specifically

* deprecate `MsgChannelOpenTry::previous_channel_id` field

* ics20: refactor on_chan_open_init

* Remove `version` field of `on_chan_open_try()`

* ICS20: inline on_chan_open_init

* ics20: on_chan_open_try

* ics20: refactor rest of handshake callbacks

* fmt

* ics26_routing: Derive Hash for ModuleId (#179)

* ics26_routing: Derive Hash for ModuleId

Signed-off-by: Kevin Ji <1146876+kevinji@users.noreply.github.com>

* Add changelog for #179

Signed-off-by: Kevin Ji <1146876+kevinji@users.noreply.github.com>
Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>

* Release v0.20.0 (#186)

* release v0.20.0

* bump crate version to 0.20.0

* update CONTRIBUTING

* CONTRIBUTING change

* Update ibc-proto-rs to v0.21.0

* Update CONTRIBUTING.md

* Connection proofs refactor (#170)

* conn_open_confirm refactor

* conn_open_ack handler

* disable mbt

* ConnOpenConfirm handler

Remove connection::verify.rs

* conn_open_ack: readability

* remove ConnectionOpenTry::with_previous_connection_id

* Make previous_connection_id deprecated

* remove mbt

* Remove hold_oldest_height check

* changelog

* Output logs for handlers again

* conn_open_ack: apply naming convention

* conn_open_try apply naming convention

* conn_open_init apply naming convention

* conn_open_try naming fix

* conn_open_confirm apply naming convention

* Document naming convention

* fmt

* comments

* Update crates/ibc/src/core/ics03_connection/handler.rs

Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>
Signed-off-by: Philippe Laferrière <plafer@protonmail.com>

* Move naming convention docstring

* fix

* update deprecated annotation

Signed-off-by: Philippe Laferrière <plafer@protonmail.com>
Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>

* changelog

* ics20 on_chan_open_init tests

* ics20 on_chan_open_try tests

* update deprecation versions

* update comment with issue

* fmt

* use ModuleExtras::empty()

Signed-off-by: Kevin Ji <1146876+kevinji@users.noreply.github.com>
Signed-off-by: Philippe Laferrière <plafer@protonmail.com>
Co-authored-by: Kevin Ji <1146876+kevinji@users.noreply.github.com>
Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>
shuoer86 pushed a commit to shuoer86/ibc-rs that referenced this pull request Nov 4, 2023
@dependabot @crodriguezvega
Bump semver from 6.3.0 to 6.3.1 in /js (informalsystems#179)
b8c6d05 
Bumps [semver](https://github.com/npm/node-semver) from 6.3.0 to 6.3.1.
- [Release notes](https://github.com/npm/node-semver/releases)
- [Changelog](https://github.com/npm/node-semver/blob/v6.3.1/CHANGELOG.md)
- [Commits](npm/node-semver@v6.3.0...v6.3.1)

---
updated-dependencies:
- dependency-name: semver
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
@kevinji @hu55a1n1
ics26_routing: Derive Hash for ModuleId (#179)
bdd70c3 
* ics26_routing: Derive Hash for ModuleId

Signed-off-by: Kevin Ji <1146876+kevinji@users.noreply.github.com>

* Add changelog for #179

Signed-off-by: Kevin Ji <1146876+kevinji@users.noreply.github.com>
Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
@plafer @kevinji @hu55a1n1
Remove crossing hellos from Channel (#181)
7e49b26 
* variable naming in ics26

* Rework ics26 channel handler

* cleanup Ics4ChannelMsg handler

* channel callbacks now return ModuleExtras

* channel_dispatch now returns a `MsgReceipt`

* remove crossing hellos logic from chan_open_try

Also removes tests that tested that logic specifically

* deprecate `MsgChannelOpenTry::previous_channel_id` field

* ics20: refactor on_chan_open_init

* Remove `version` field of `on_chan_open_try()`

* ICS20: inline on_chan_open_init

* ics20: on_chan_open_try

* ics20: refactor rest of handshake callbacks

* fmt

* ics26_routing: Derive Hash for ModuleId (#179)

* ics26_routing: Derive Hash for ModuleId

Signed-off-by: Kevin Ji <1146876+kevinji@users.noreply.github.com>

* Add changelog for #179

Signed-off-by: Kevin Ji <1146876+kevinji@users.noreply.github.com>
Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>

* Release v0.20.0 (#186)

* release v0.20.0

* bump crate version to 0.20.0

* update CONTRIBUTING

* CONTRIBUTING change

* Update ibc-proto-rs to v0.21.0

* Update CONTRIBUTING.md

* Connection proofs refactor (#170)

* conn_open_confirm refactor

* conn_open_ack handler

* disable mbt

* ConnOpenConfirm handler

Remove connection::verify.rs

* conn_open_ack: readability

* remove ConnectionOpenTry::with_previous_connection_id

* Make previous_connection_id deprecated

* remove mbt

* Remove hold_oldest_height check

* changelog

* Output logs for handlers again

* conn_open_ack: apply naming convention

* conn_open_try apply naming convention

* conn_open_init apply naming convention

* conn_open_try naming fix

* conn_open_confirm apply naming convention

* Document naming convention

* fmt

* comments

* Update crates/ibc/src/core/ics03_connection/handler.rs

Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>
Signed-off-by: Philippe Laferrière <plafer@protonmail.com>

* Move naming convention docstring

* fix

* update deprecated annotation

Signed-off-by: Philippe Laferrière <plafer@protonmail.com>
Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>

* changelog

* ics20 on_chan_open_init tests

* ics20 on_chan_open_try tests

* update deprecation versions

* update comment with issue

* fmt

* use ModuleExtras::empty()

Signed-off-by: Kevin Ji <1146876+kevinji@users.noreply.github.com>
Signed-off-by: Philippe Laferrière <plafer@protonmail.com>
Co-authored-by: Kevin Ji <1146876+kevinji@users.noreply.github.com>
Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@hu55a1n1 hu55a1n1 hu55a1n1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kevinji @DaviRain-Su @hu55a1n1