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.

Sign up for GitHub

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

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 [email protected]

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

@DaviRain-Su
Copy link
Contributor

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

@hu55a1n1
Copy link
Contributor

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
Contributor

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
Contributor

@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
Contributor

@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 cosmos: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 <[email protected]>

* Add changelog for #179

Signed-off-by: Kevin Ji <[email protected]>
Co-authored-by: Shoaib Ahmed <[email protected]>
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 <[email protected]>

* Add changelog for #179

Signed-off-by: Kevin Ji <[email protected]>
Co-authored-by: Shoaib Ahmed <[email protected]>

* 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 <[email protected]>
Signed-off-by: Philippe Laferrière <[email protected]>

* Move naming convention docstring

* fix

* update deprecated annotation

Signed-off-by: Philippe Laferrière <[email protected]>
Co-authored-by: Shoaib Ahmed <[email protected]>

* 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 <[email protected]>
Signed-off-by: Philippe Laferrière <[email protected]>
Co-authored-by: Kevin Ji <[email protected]>
Co-authored-by: Shoaib Ahmed <[email protected]>
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 (cosmos#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] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Carlos Rodriguez <[email protected]>
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 <[email protected]>

* Add changelog for #179

Signed-off-by: Kevin Ji <[email protected]>
Co-authored-by: Shoaib Ahmed <[email protected]>
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 <[email protected]>

* Add changelog for #179

Signed-off-by: Kevin Ji <[email protected]>
Co-authored-by: Shoaib Ahmed <[email protected]>

* 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 <[email protected]>
Signed-off-by: Philippe Laferrière <[email protected]>

* Move naming convention docstring

* fix

* update deprecated annotation

Signed-off-by: Philippe Laferrière <[email protected]>
Co-authored-by: Shoaib Ahmed <[email protected]>

* 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 <[email protected]>
Signed-off-by: Philippe Laferrière <[email protected]>
Co-authored-by: Kevin Ji <[email protected]>
Co-authored-by: Shoaib Ahmed <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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