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.

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

Use hashed description #884

Merged
merged 10 commits into from
Jul 25, 2023
Merged

Use hashed description #884

merged 10 commits into from
Jul 25, 2023

Conversation

alistair-singh
Copy link
Contributor

@alistair-singh alistair-singh force-pushed the alistair/use-hashed-description branch from 2c4d357 to a2f205c Compare July 20, 2023 08:19
@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Patch coverage: 40.00% and project coverage change: -0.04 ⚠️

Comparison is base (6bceb86) 69.80% compared to head (d92a510) 69.76%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #884      +/-   ##
==========================================
- Coverage   69.80%   69.76%   -0.04%     
==========================================
  Files          47       47              
  Lines        1679     1677       -2     
  Branches       58       58              
==========================================
- Hits         1172     1170       -2     
  Misses        489      489              
  Partials       18       18              
Flag Coverage Δ
rust 70.14% <40.00%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
parachain/primitives/router/src/inbound/mod.rs 5.45% <40.00%> (-1.69%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

parachain/primitives/router/src/inbound/mod.rs Outdated Show resolved Hide resolved
2,
(
GlobalConsensus(Ethereum { chain_id }),
AccountKey20 { network: None, key: *origin.as_fixed_bytes() },
Copy link
Contributor

Choose a reason for hiding this comment

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

When would we set a network id in an account key?

Copy link
Contributor

@yrong yrong Jul 21, 2023

Choose a reason for hiding this comment

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

Guess already set in GlobalConsensus(network) above so can just ignore here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An example of where we "could" use network id is if we are specifying an ERC20 token contract on an EVM based parachain and potentially transferring it over the bridge and we would want a way to specify its Network as Kusama so we could handle it specially in some way. But I say "could" here because it's hypothetical.

If you look at some places in the source code, we often ignore network id. Such as here.
There are also some places where it makes sense to validate it if present like here.

In this case yrong is correct in that it is set in GlobalConsensus so we can set it as None on the key itself.

@alistair-singh alistair-singh force-pushed the alistair/use-hashed-description branch from c3fcb2e to 41a49de Compare July 24, 2023 07:18
@alistair-singh alistair-singh merged commit d26678d into main Jul 25, 2023
4 checks passed
@alistair-singh alistair-singh deleted the alistair/use-hashed-description branch July 25, 2023 08:59
alistair-singh added a commit that referenced this pull request Jul 27, 2023
alistair-singh added a commit that referenced this pull request Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants