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

feat: add super trait for block number #2334

Merged
merged 15 commits into from
Dec 14, 2023
Merged

feat: add super trait for block number #2334

merged 15 commits into from
Dec 14, 2023

Conversation

yjhmelody
Copy link
Contributor

And also related to a subxt PR paritytech/subxt#1265

@yjhmelody yjhmelody requested a review from a team November 15, 2023 08:16
@yjhmelody yjhmelody mentioned this pull request Nov 15, 2023
Merged
@paritytech-review-bot paritytech-review-bot bot requested a review from a team November 15, 2023 08:16
ggwpez
ggwpez requested changes Nov 15, 2023
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

We cannot change this for downstream convenience.

@yjhmelody
Copy link
Contributor Author

yjhmelody commented Nov 16, 2023 •
edited
Loading

@ggwpez I find archive_unstable_hash_by_height use Into<u64> for block number. Should I add it back? Or generalize the api? But a bridge test use u128 which could not impl this trait.

@ggwpez
Copy link
Member

ggwpez commented Nov 27, 2023

I have no clue what you are trying to do here. We should not need to change this trait here for any downstream project. They can just artificially restrict it even more if needed.

Currently we have this:

+ Into<U256>
+ TryFrom<U256>

And now you add TryFrom<u64>? It is possible to convert a u64 first to a U256, so i really dont see the use in this.

@yjhmelody
Copy link
Contributor Author

@ggwpez Ok i will remove TryFrom<u64>

@yjhmelody
Copy link
Contributor Author

Please take a look. Thx! @ggwpez

@bkchr bkchr added the R0-silent Changes should not be mentioned in any release notes label Dec 13, 2023
@bkchr bkchr merged commit 2cee874 into paritytech:master Dec 14, 2023
112 of 116 checks passed
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
@yjhmelody
feat: add super trait for block number (paritytech#2334)
17e7e2f 
And also related to a subxt PR
paritytech/subxt#1265
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xlc xlc xlc left review comments

@ggwpez ggwpez ggwpez approved these changes

@bkchr bkchr bkchr approved these changes

Assignees
No one assigned
Labels
R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yjhmelody @ggwpez @xlc @bkchr