feat: extend the maximum number of characters that can be decoded to 200 characters by kokehiM0chi · Pull Request #159 · Finschia/ostracon

kokehiM0chi · 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


 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

 		{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

 		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

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

kokehiM0chi 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

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

kokehiM0chi 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

kokehiM0chi requested review from egonspace, tnasu and torao 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

kokehiM0chi 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

kokehiM0chi 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

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

kokehiM0chi 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)

		@@ -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

Conversation

kokehiM0chi commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

codecov bot commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokehiM0chi Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokehiM0chi Jan 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torao Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokehiM0chi Jan 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torao left a comment

Choose a reason for hiding this comment

Uh oh!

kokehiM0chi commented Jan 6, 2021 •

edited

Loading

codecov bot commented Jan 6, 2021 •

edited

Loading

kokehiM0chi Jan 7, 2021 •

edited

Loading

kokehiM0chi Jan 18, 2021 •

edited

Loading

torao Jan 21, 2021 •

edited

Loading

kokehiM0chi Jan 18, 2021 •

edited

Loading

torao Jan 21, 2021 •

edited

Loading