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

Address encoding of block.coinbase_account #70

Closed
graup opened this issue Sep 6, 2019 · 10 comments
Closed

Address encoding of block.coinbase_account #70

graup opened this issue Sep 6, 2019 · 10 comments
Assignees
Labels
bug Something isn't working

Comments

@graup
Copy link
Member

graup commented Sep 6, 2019

block.coinbase_account should be a b58check encoded address.

Current result: eZDSP5tk3jz49yjxiF2XMkfou3VQtDRxVow1PNPuz3Qe

Expected result: AmLrV7tg69KE5ZQehkjGiA7yJyDxJ25uyF96PMRptfzwozhAJbaK

"coinbase_account": encode_b58(self.coinbase_account),

@hanlsin
Copy link
Member

hanlsin commented Oct 21, 2019

Hi @graup,

Sorry for late response.

The coinbase account is printing base58.Encode, not base58check.

Where did you find to use base58check for the coinbase account?

https://github.com/aergoio/aergo/blob/35879e34e615f2498a31381ffa97160e98211379/cmd/aergocli/util/base58addr.go#L243

@hanlsin hanlsin self-assigned this Oct 21, 2019
@hanlsin hanlsin added the bug Something isn't working label Oct 21, 2019
@graup
Copy link
Member Author

graup commented Oct 22, 2019

@hanlsin Exactly, and that is the problem. It SHOULD be base58check, because it is an address.

@hanlsin
Copy link
Member

hanlsin commented Oct 22, 2019

In that case, it's not only base58check, but also AddressVersion + base58check(addr). Right?

I will report it to Aergo CLI and make it right.

@graup
Copy link
Member Author

graup commented Oct 22, 2019

More like base58check(addr, AddressVersion), but yes. Thanks

@graup
Copy link
Member Author

graup commented Oct 22, 2019

Cant you just replace the line with types.EncodeAddress(self.coinbase_account)?

@graup
Copy link
Member Author

graup commented Oct 22, 2019

Sorry, I misunderstood your previous message. I didn't realize this bug (if I'm right) was also in aergocli. Let's fix it there first!

@hanlsin
Copy link
Member

hanlsin commented Oct 22, 2019

I reported it for aergocli as a question at first, because I don't know the decision about it.

aergoio/aergo#91

And, you are right. Just, in aergocli, replacing

https://github.com/aergoio/aergo/blob/35879e34e615f2498a31381ffa97160e98211379/cmd/aergocli/util/base58addr.go#L243

to out.Header.CoinbaseAccount = types.EncodeAddress(b.GetHeader().GetCoinbaseAccount())

is the simple answer.

@hanlsin
Copy link
Member

hanlsin commented Oct 22, 2019

More like base58check(addr, AddressVersion), but yes. Thanks

Yes. If I may allow to fix it little bit, then it would be base58check(AddressVersion, addr), because the version is in front of the address. :)

@hanlsin
Copy link
Member

hanlsin commented Oct 23, 2019

aergoio/aergo#91

In the issue above, the only base58 expression was mistake. So I will modify herapy for the next release too.

hanlsin added a commit that referenced this issue Oct 23, 2019
@hanlsin
Copy link
Member

hanlsin commented Nov 5, 2019

It is included in version 1.3.3.

@hanlsin hanlsin closed this as completed Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants