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

Implement the formatting method to return a mixed-case EIP 55 string #43

Merged
merged 5 commits into from Jan 22, 2019

Conversation

@earlbread
Copy link
Contributor

@earlbread earlbread commented Jan 19, 2019

This Implements the formatting method to return a mixed-case EIP 55 string(#33).

Copy link
Member

@longfin longfin left a comment

Thank you for patch! I've made some comments.

[Pure]
public string ToChecksumAddress()
{
var hex = ToHex();
Copy link
Member

@longfin longfin Jan 21, 2019

Choose a reason for hiding this comment

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

it is more readable to write the type explicitly If the type of the variable isn't obvious with new.

Copy link
Contributor Author

@earlbread earlbread Jan 21, 2019

Choose a reason for hiding this comment

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

Thank you, I'll fix it.

public string ToChecksumAddress()
{
var hex = ToHex();
var bytes = Encoding.ASCII.GetBytes(hex);
Copy link
Member

@longfin longfin Jan 21, 2019

Choose a reason for hiding this comment

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

Instead of re-parsing, we can call ToByteArray() here.

Copy link
Contributor Author

@earlbread earlbread Jan 21, 2019

Choose a reason for hiding this comment

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

I think the results are different. the bytes in the code is 40 bytes and the result of ToByteArray() is 20 bytes.

Copy link
Member

@longfin longfin Jan 21, 2019

Choose a reason for hiding this comment

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

My bad, I've mistaken.

@dahlia dahlia added this to the 0.1.0 milestone Jan 21, 2019
Copy link
Member

@dahlia dahlia left a comment

Thanks for your first contribution! I left a comment.

/// <returns>A <c>0x</c>-prefixed Mixed-case checksum address of 42
/// letters that represent this <see cref="Address"/>.</returns>
[Pure]
public string ToChecksumAddress()
Copy link
Member

@dahlia dahlia Jan 21, 2019

Choose a reason for hiding this comment

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

I believe mixed-case hexadecimals should be encouraged and the default. In other words, ToHex() method should return a mixed-case hexadecimals unless it's explicitly off, e.g., ToHex(checksum: false). (Or such option maybe unnecessary from the outset, because calling .ToLower() is trivial.) The same goes for ToString().

@longfin @kijun Opinions?

Copy link
Member

@longfin longfin Jan 21, 2019

Choose a reason for hiding this comment

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

Good point, I agree.

Copy link
Contributor Author

@earlbread earlbread Jan 21, 2019

Choose a reason for hiding this comment

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

I agree. I'll fix ToHex() method instead of creating ToChecksumAddress()

@earlbread
Copy link
Contributor Author

@earlbread earlbread commented Jan 22, 2019

I applied code reviews and rebased the branch.

Copy link
Member

@dahlia dahlia left a comment

I left only few trivial comments. Thanks!

string hashHex = ByteUtil.Hex(hash);
string address = string.Empty;

for (var i = 0; i < hex.Length; i++)
Copy link
Member

@dahlia dahlia Jan 22, 2019

Choose a reason for hiding this comment

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

As string implements IEnumerable<char>, this can be adjusted to use foreach instead.

Copy link
Member

@longfin longfin Jan 22, 2019

Choose a reason for hiding this comment

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

It seems to use i as the index of hexHash.

Copy link
Member

@dahlia dahlia Jan 22, 2019

Choose a reason for hiding this comment

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

@longfin Fair enough. This should be as is. @earlbread Nevermind!

/// Gets a hexadecimal string of 40 letters that represent this
/// <see cref="Address"/>.
/// Gets a mixed-case hexadecimal string of 40 letters that represent
/// this <see cref="Address"/>.
/// </summary>
Copy link
Member

@dahlia dahlia Jan 22, 2019

Choose a reason for hiding this comment

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

Could you append a mention that a returned hexadecimal string follows EIP 55?

Copy link
Contributor Author

@earlbread earlbread Jan 22, 2019

Choose a reason for hiding this comment

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

I rebased on master and added a mention about EIP 55!

@dahlia
Copy link
Member

@dahlia dahlia commented Jan 22, 2019

Note that a build error during pushing docs was fixed by PR #45. You should rebase this PR on the current master again (20386c5 at least).

dahlia
dahlia approved these changes Jan 22, 2019
@dahlia dahlia merged commit b18c0ca into planetarium:master Jan 22, 2019
1 check passed
@dahlia
Copy link
Member

@dahlia dahlia commented Jan 22, 2019

Thanks for your contribution!

@earlbread earlbread deleted the eip55 branch Jan 22, 2019
@longfin longfin mentioned this pull request Jan 23, 2019
@dahlia dahlia mentioned this pull request Jan 23, 2019
2 tasks
limebell pushed a commit to limebell/libplanet that referenced this issue Jul 7, 2021
…-fields-in-shop-state

State/remove useless fields in shop state
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants