feat: extend the maximum number of characters that can be decoded to 200 characters #159

Kynea0b · 2021-01-06T09:34:00Z

Description

For contributor use:

Wrote tests

codecov · 2021-01-06T10:56:28Z

Codecov Report

Merging #159 (2dcd9df) into feat/bech32plus (a01efd2) will increase coverage by 0.16%.
The diff coverage is 90.54%.

@@                 Coverage Diff                 @@
##           feat/bech32plus     #159      +/-   ##
===================================================
+ Coverage            62.52%   62.69%   +0.16%     
===================================================
  Files                  283      284       +1     
  Lines                26475    26617     +142     
===================================================
+ Hits                 16554    16688     +134     
- Misses                8576     8580       +4     
- Partials              1345     1349       +4

Impacted Files	Coverage Δ
types/protobuf.go	`81.81% <ø> (ø)`
crypto/composite/composite.go	`96.07% <90.00%> (-3.93%)`	⬇️
libs/bech32/bech32plus.go	`90.08% <90.08%> (ø)`
crypto/encoding/amino/amino.go	`95.74% <100.00%> (ø)`
libs/bech32/bech32.go	`53.84% <100.00%> (ø)`
lite2/trust_options.go	`18.18% <0.00%> (-15.16%)`	⬇️
consensus/state.go	`77.93% <0.00%> (-0.10%)`	⬇️
consensus/reactor.go	`79.86% <0.00%> (+0.11%)`	⬆️
blockchain/v0/pool.go	`78.98% <0.00%> (+0.31%)`	⬆️
... and 5 more

libs/bech32/bech32plus.go

tnasu · 2021-01-06T11:33:15Z

libs/bech32/bech32plus.go

+// Decode decodes a bech32 encoded string, returning the human-readable
+// part and the data part excluding the checksum.
+func Decode(bech string) (string, []byte, error) {
+	// The maximum allowed length for a bech32 string is 90. It must also


a bech32 string is 90
Should update 90

Thank you. I will fix．

tnasu · 2021-01-06T11:35:18Z

crypto/composite/composite.go

-	msg := bytes.NewBuffer(pk.SignKey.Bytes())
-	msg.Write(pk.VrfKey.Bytes())
-	return msg.Bytes()
+	bz, err := cdc.MarshalBinaryBare(pk)


Why are we using bytes.NewBuffer?

FYI: https://github.com/line/tendermint/blob/develop/docs/architecture/adr-015-crypto-encoding.md

Context

We must standardize our method for encoding public keys and signatures on chain. Currently we amino encode the public keys and signatures. The reason we are using amino here is primarily due to ease of support in parsing for other languages. We don't need its upgradability properties in cryptosystems, as a change in the crypto that requires adapting the encoding, likely warrants being deemed a new cryptosystem. (I.e. using new public parameters)

Thank you. It was used to identify it as a unique value as a [] byte array inside tendermint. However, according to the specifications of tendermint, it seems correct to use amino encoding.

tnasu · 2021-01-06T11:36:09Z

crypto/composite/composite.go

@@ -67,7 +87,7 @@ func (sk PrivKeyComposite) Identity() crypto.PrivKey {
 }

 func (sk PrivKeyComposite) Bytes() []byte {
-	return sk.Identity().Bytes()
+	return cdc.MustMarshalBinaryBare(sk)


Why are we using sk.Identity().Bytes()?

As I answered above, the reason is the same as PubKeyComposite.

tnasu · 2021-01-06T11:38:05Z

libs/bech32/bech32plus.go

@@ -0,0 +1,255 @@
+// Copyright (c) 2017 The btcsuite developers


Should we add our copylight?

Thank you. No. I will delete it.

Oh, don't we need copyright? Really?

The ISC license is a non-copyleft type license. In this case, the copyright is not inherited, so the copyright notice may or may not be present, but just in case, it is attached as it is.

Yes, we should. The above was a bit inaccurate. Sorry. It seems correct to describe the license for LINE. Since the license name has not been decided now, I treated it as TODO.

Here, the non-copyleft license means that license inheritance doesn't apply. The "license inheritance" means that the source code to be copied must be under the identical license. However, license inheritance and copyright notice obligations are different concepts.

Therefore, we don't need to inherit the ISC license, but we do need to leave the copyright notice according to the ISC license.

tnasu · 2021-01-06T11:38:22Z

libs/bech32/bech32plus_test.go

@@ -0,0 +1,72 @@
+// Copyright (c) 2017 The btcsuite developers


Should we add our copylight?

Same as above.

Yes, we should. It seems correct to describe the license for LINE. Since the license name has not been decided now, I treated it as TODO.

tnasu · 2021-01-06T11:38:29Z

libs/bech32/exampleplus_test.go

@@ -0,0 +1,49 @@
+// Copyright (c) 2017 The btcsuite developers


Should we add our copylight?

Same as above.

Yes, we should. It seems correct to describe the license for LINE. Since the license name has not been decided now, I treated it as TODO.

libs/bech32/bech32plus.go

…btcsuite/btcutil/bech32/bech32.go

tnasu · 2021-01-18T08:54:44Z

crypto/encoding/amino/encode_test.go

@@ -155,6 +168,7 @@ func TestPubkeyAminoName(t *testing.T) {
 		{ed25519.PubKeyEd25519{}, ed25519.PubKeyAminoName, true},
 		{sr25519.PubKeySr25519{}, sr25519.PubKeyAminoName, true},
 		{secp256k1.PubKeySecp256k1{}, secp256k1.PubKeyAminoName, true},
+		{composite.PubKeyComposite{}, composite.PubKeyAminoName, true},


No need to add bls?

Thank you. I will add bls.

I fixed. PTAL.

tnasu · 2021-01-18T08:59:24Z

types/genesis_test.go

+	assert.NotNil(t, genDoc.ConsensusParams, "expected consensus params to be filled in")
+
+	// check validator's address is filled
+	assert.NotNil(t, genDoc.Validators[0].Address, "expected validator's address to be filled in")


Why don't you check the composite fields? Is it enough to check Address since you add this test case?

Yes, it is. The address is generated from the pubkey. Therefore, the address check includes the pubkey check.

I mean that we should check the type whether composite or not.

Thank you. I added the test case whether composite or not.

tnasu · 2021-01-19T02:41:04Z

crypto/composite/composite_test.go

@@ -253,7 +254,7 @@ func TestEnvironmentalCompatibility(t *testing.T) {
 		// compare addresses to assumed value
 		compositePrivKey := composite.NewPrivKeyComposite(blsPrivKey, ed25519PrivKey)
 		compositePubKey := compositePrivKey.PubKey()
-		address, err := hex.DecodeString("7A68265205CB115AE35A13515C423F1721E87BB4")
+		address, err := hex.DecodeString(strings.ToUpper("72dd758835404175940f698cf3ddc29dd0d04afa"))


nit: No need to do ToUpper?

Thank you. That's right.

I fixed. PTAL.

torao

LGTM

torao · 2021-01-21T08:41:58Z

libs/bech32/bech32plus.go

@@ -0,0 +1,255 @@
+// Copyright (c) 2017 The btcsuite developers


Here, the non-copyleft license means that license inheritance doesn't apply. The "license inheritance" means that the source code to be copied must be under the identical license. However, license inheritance and copyright notice obligations are different concepts.

Therefore, we don't need to inherit the ISC license, but we do need to leave the copyright notice according to the ISC license.

feat: extend the maximum number of characters that can be decoded to 200 characters (#159)

tnasu reviewed Jan 6, 2021

View reviewed changes

Kynea0b changed the base branch from develop to feat/bech32plus January 7, 2021 01:52

Kynea0b self-assigned this Jan 7, 2021

tnasu reviewed Jan 8, 2021

View reviewed changes

libs/bech32/bech32plus.go Show resolved Hide resolved

feat: move into tendermint/tendermint/libs/bech32/bech32plus.go from …

bb1bca9

…btcsuite/btcutil/bech32/bech32.go

Kynea0b requested review from torao, tnasu and egonspace and removed request for torao, tnasu and egonspace January 12, 2021 07:13

Kynea0b changed the title ~~feat: Extend the maximum number of characters that can be decoded to 200 characters~~ feat: extend the maximum number of characters that can be decoded to 200 characters Jan 18, 2021

Kynea0b requested review from tnasu, torao and egonspace January 18, 2021 08:52

tnasu reviewed Jan 18, 2021

View reviewed changes

tnasu reviewed Jan 19, 2021

View reviewed changes

tnasu previously approved these changes Jan 20, 2021

View reviewed changes

Kynea0b added 4 commits January 21, 2021 15:29

feat: add LIMIT for decode to bech32plus and call this in bech32.go

c8120db

fix: change bytes-codec to amino-codec

c8f8c44

feat: add composite key and bls key encode test

0f2a0a3

feat: add genesis_test.go using composite pub_key

2dcd9df

Kynea0b dismissed tnasu’s stale review via 2dcd9df January 21, 2021 06:29

tnasu approved these changes Jan 21, 2021

View reviewed changes

torao approved these changes Jan 21, 2021

View reviewed changes

Kynea0b merged commit bda3271 into Finschia:feat/bech32plus Jan 25, 2021

Kynea0b added a commit that referenced this pull request Feb 18, 2021

Merge pull request #185 from line/feat/bech32plus

20e5c02

feat: extend the maximum number of characters that can be decoded to 200 characters (#159)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: extend the maximum number of characters that can be decoded to 200 characters #159

feat: extend the maximum number of characters that can be decoded to 200 characters #159

Kynea0b commented Jan 6, 2021 •

edited

Loading

codecov bot commented Jan 6, 2021 •

edited

Loading

tnasu Jan 6, 2021

Kynea0b Jan 7, 2021

tnasu Jan 6, 2021

tnasu Jan 8, 2021

Kynea0b Jan 8, 2021

tnasu Jan 6, 2021

Kynea0b Jan 8, 2021

tnasu Jan 6, 2021

Kynea0b Jan 7, 2021 •

edited

Loading

tnasu Jan 7, 2021

Kynea0b Jan 12, 2021

Kynea0b Jan 18, 2021 •

edited

Loading

torao Jan 21, 2021 •

edited

Loading

tnasu Jan 6, 2021

Kynea0b Jan 7, 2021

Kynea0b Jan 18, 2021 •

edited

Loading

tnasu Jan 6, 2021

Kynea0b Jan 7, 2021

Kynea0b Jan 18, 2021

tnasu Jan 18, 2021

Kynea0b Jan 19, 2021

Kynea0b Jan 20, 2021

tnasu Jan 18, 2021

Kynea0b Jan 20, 2021

tnasu Jan 20, 2021

Kynea0b Jan 20, 2021

tnasu Jan 19, 2021

Kynea0b Jan 19, 2021

Kynea0b Jan 20, 2021

torao left a comment

torao Jan 21, 2021 •

edited

Loading

		@@ -0,0 +1,255 @@
		// Copyright (c) 2017 The btcsuite developers

		@@ -0,0 +1,72 @@
		// Copyright (c) 2017 The btcsuite developers

		@@ -0,0 +1,49 @@
		// Copyright (c) 2017 The btcsuite developers

feat: extend the maximum number of characters that can be decoded to 200 characters #159

feat: extend the maximum number of characters that can be decoded to 200 characters #159

Conversation

Kynea0b commented Jan 6, 2021 • edited Loading

Description

codecov bot commented Jan 6, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kynea0b Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kynea0b Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

torao Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kynea0b Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torao left a comment

Choose a reason for hiding this comment

torao Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Kynea0b commented Jan 6, 2021 •

edited

Loading

codecov bot commented Jan 6, 2021 •

edited

Loading

Kynea0b Jan 7, 2021 •

edited

Loading

Kynea0b Jan 18, 2021 •

edited

Loading

torao Jan 21, 2021 •

edited

Loading

Kynea0b Jan 18, 2021 •

edited

Loading

torao Jan 21, 2021 •

edited

Loading