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

feat: Basic support for H160 and H256 accounts. #596

Merged
merged 10 commits into from
Jul 27, 2021

Conversation

crystalin
Copy link
Contributor

This is a basic support got H160 accounts (like in Moonbeam/Moonriver) substrate chains.
I suspect we could pass an argument to validateAddressMiddleware to decide which type to support instead of relying on the 0x prefix, but I'm not too familiar with the project.

What is your suggestion ?

@TarikGul TarikGul added the I8 - Enhancement Additional feature request label Jul 3, 2021
@TarikGul TarikGul requested review from emostov and dvdplm July 3, 2021 17:55
@TarikGul
Copy link
Member

TarikGul commented Jul 3, 2021

@crystalin Thank you for the PR! I will give it a thorough review and response come Monday.

@emostov
Copy link
Contributor

emostov commented Jul 3, 2021

We need to update tests and make sure to have both success and failure tests for standard substrate addresses and H160.

Copy link
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

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

thanks for the contribution - LGTM!

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

Seems like this could be done in a better way, but I could be wrong.

src/middleware/validate/validateAddressMiddleware.ts Outdated Show resolved Hide resolved
src/middleware/validate/validateAddressMiddleware.ts Outdated Show resolved Hide resolved
src/middleware/validate/validateAddressMiddleware.ts Outdated Show resolved Hide resolved
src/middleware/validate/validateAddressMiddleware.ts Outdated Show resolved Hide resolved
@crystalin
Copy link
Contributor Author

@dvdplm , I tried to improve the logic, please have a look

@TarikGul
Copy link
Member

TarikGul commented Jul 5, 2021

LGTM, I will approve once the lint check is fixed (yarn lint --fix). Thanks again for the PR.

NOTE: I tested this myself against a moonriver node, and everything worked as expected.

@TarikGul TarikGul mentioned this pull request Jul 5, 2021
@TarikGul TarikGul changed the title Basic support got H160 accounts feat: Basic support for H160 and H256 accounts. Jul 5, 2021
Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

@crystalin This will make it into the next Sidecar release v8.1.0. @dvdplm Once you approve the changes made in response to your suggestions I will get this updated into the next Sidecar release for either tonight or tomorrow morning.

@TarikGul
Copy link
Member

TarikGul commented Jul 5, 2021

This will make it into the next Sidecar release v8.1.0.

This was premature insight of mine.

So we actually had a quick conversation about moving forward with this PR. We are actually going to leave it out of the next release to give this a second audit, we believe there might be a better way to do this, but just need to double check some alternatives.

Moving forward with the 8.0.1 release and once this is merged we can always do another release.

@crystalin
Copy link
Contributor Author

@TarikGul it is fine for us now, we are hosting our custom version at the moment

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lgtm, thank you @crystalin for humoring me. :)

@emostov
Copy link
Contributor

emostov commented Jul 26, 2021

What is the status of this?

@TarikGul
Copy link
Member

@emostov Im happy to get this merged in.

@TarikGul
Copy link
Member

TarikGul commented Jul 26, 2021

My main concern and delay was wondering if there was a better alternative to accepting a set of addresses within our middleware, but I think for now this is totally acceptable.

@emostov
Copy link
Contributor

emostov commented Jul 26, 2021

@TarikGul - yeah I wouldn't say I have a full picture of all the different address schemes, but from what I understand this solution should additionally except ethereum addresses on top of what we already except. If for some reason we found out accepting both is not desirable I suppose we could add some config option. Not sure what other ways we could do this?

@TarikGul
Copy link
Member

TarikGul commented Jul 26, 2021

@emostov Yea I agree with everything you said. I am going to do some last minute sanity checks, and if everything checks out again (which it should), I'll merge this tonight, and have it in tomorrow's release

@TarikGul
Copy link
Member

TarikGul commented Jul 27, 2021

Merging. @crystalin Thank you for the PR.

@TarikGul TarikGul merged commit bddc2a2 into paritytech:master Jul 27, 2021
@crystalin crystalin deleted the crystalin-h160-account branch July 27, 2021 02:40
@TarikGul TarikGul mentioned this pull request Jul 27, 2021
@jsdw jsdw mentioned this pull request Jul 27, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I8 - Enhancement Additional feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants