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

Add support for bech32 in genesis file #362

Closed
ethanfrey opened this issue Feb 27, 2019 · 7 comments · Fixed by #415
Closed

Add support for bech32 in genesis file #362

ethanfrey opened this issue Feb 27, 2019 · 7 comments · Fixed by #415
Assignees

Comments

@ethanfrey
Copy link
Contributor

ethanfrey commented Feb 27, 2019

Builds on #347

Also allow the bech32: prefix to be parsed:

We can use tendermint bech32 implementation as an example and use the implementation from https://github.com/btcsuite/btcutil/tree/master/bech32

We should also verify the library matches the values we have with iov-core (use some standard encode/decode test vectors)

@husio
Copy link
Contributor

husio commented Mar 5, 2019

When importing a bech32 implementation I think we should test it ourselves as well to ensure the quality. We can probably copy tests from iov-core.
We have used https://github.com/ltcsuite/ltcutil in the past and the implementation was wrong. I have wasted time trying to find an issue in my code assuming that the bech32 implementation was correct.

@webmaster128 created a bug report for the tlcutil repository but I can see that they have solved the problem by disabling the issues 😂

@webmaster128
Copy link
Contributor

@husio I think they don't care much about code they inherit from https://github.com/btcsuite/btcutil and don't use. Maybe it is worth fixing the btcsuite/btcutil implementation and use it.

@ethanfrey ethanfrey added the ready label Mar 5, 2019
@husio husio self-assigned this Mar 14, 2019
@husio
Copy link
Contributor

husio commented Mar 14, 2019

Blocked by btcsuite/btcutil#137 unless we will use different bech32 decoder implementation.

Initial work done in https://github.com/iov-one/weave/tree/bech32_support_issue_362

@husio husio added blocked Work on the issue cannot be continue before another problem is solved. and removed ready labels Mar 14, 2019
husio added a commit that referenced this issue Mar 14, 2019
@ethanfrey
Copy link
Contributor Author

Looks like that library is a no-go for now. I was digging in to see if the bech32 c utility was reliable (or maybe that one had errors). Seems this uses code pulled from the reference implementation.

Digging for test vectors to validate one over the other, I found a list of reference implementations in many languages, including go. And this version includes a complete test suite with test vectors.

Digging in, I found no license file, but as one github user pointed out, there is a BSD license in the source itself.

Given this code has not changed in around 2 years and is a reference implementation, I would suggest copying these files (including the original copyright notice) into our repo and using that.

@husio
Copy link
Contributor

husio commented Mar 15, 2019

Copy https://github.com/sipa/bech32/tree/master/ref/go/src/bech32 as is into weave/crypto as weave/crypto/bech32.

@ethanfrey ethanfrey removed the blocked Work on the issue cannot be continue before another problem is solved. label Mar 15, 2019
@matheusd
Copy link

Decode() returns a slice where each element has 5 bits of the payload data. You need to further call ConvertBits to get the original data:

	// ...
	payloadDec, err := bech32.ConvertBits(payload, 5, 8, false)
	if err != nil {
		t.Fatal(err)
	}

	if !bytes.Equal(want, payloadDec) {
	// ...

@ethanfrey
Copy link
Contributor Author

Thanks @matheusd

I was really wondering how this implementation could differ from the standard. I guess we have two approaches that work now.

husio added a commit that referenced this issue Mar 18, 2019
Allow to specify a JSON serialized condition using bech32 encoding.

resolve #362
@husio husio mentioned this issue Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants