Skip to content
This repository has been archived by the owner on Sep 13, 2022. It is now read-only.

fix: load hrp before deserializing genesis payload to take hrp effect #405

Merged
merged 2 commits into from
Aug 10, 2020

Conversation

LycrusHamster
Copy link
Contributor

What this PR does / why we need it:

fix: load hrp before deserializing genesis payload to take hrp effect

Which docs this PR relation:

N/A

Which toolchain this PR adaption:

N/A

No Breaking Change

Special notes for your reviewer:

This is an internal bug and fix doesn't take any changes to toolchains and docs.

hrp should load firstly before deserializing genesis cause Address maybe in service genesis payload may need hrp

@yejiayu
Copy link
Contributor

yejiayu commented Aug 9, 2020

We should add unit test for this PR which avoid running failures due to some breaking changes in future

@LycrusHamster
Copy link
Contributor Author

We should add unit test for this PR which avoid running failures due to some breaking changes in future

So as I think. The problem is not to add test, but where to add?

@yejiayu
Copy link
Contributor

yejiayu commented Aug 9, 2020

We should add unit test for this PR which avoid running failures due to some breaking changes in future

So as I think. The problem is not to add test, but where to add?

The point of testing is to match field, so i think it's just a matter of ensuring that the serialized Metadata json string can read the fields.

Can we add it to types/metadata.rs?

@muta-robot muta-robot added size/M and removed size/S labels Aug 9, 2020
@LycrusHamster
Copy link
Contributor Author

We should add unit test for this PR which avoid running failures due to some breaking changes in future

So as I think. The problem is not to add test, but where to add?

The point of testing is to match field, so i think it's just a matter of ensuring that the serialized Metadata json string can read the fields.

Can we add it to types/metadata.rs?

The hrp can only be set once, which means if I set hrp to sth. like 'meow', the following address's hrp have to be 'meow'
However cargo test is designed to run parelly and each test case must not relate to others.
I don't wan't to set test thread to 1.
And if I set hrp, other tests shall fail.

Thus.....you see the 'crippled' test cases...

@yejiayu
Copy link
Contributor

yejiayu commented Aug 10, 2020

/lgtm

@muta-robot muta-robot added the lgtm #8ef42e label Aug 10, 2020
@muta-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yejiayu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@muta-robot muta-robot merged commit 828e6d5 into nervosnetwork:master Aug 10, 2020
@LycrusHamster LycrusHamster deleted the fix-hrp branch August 10, 2020 08:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants