fix: apply types/address.go from [email protected] #603

ulbqb · 2022-07-21T04:42:27Z

Description

This PR enable to apply types/address.go from [email protected].

Major changes:

types.AccAddress, types.ValAddress and types.ConsAddress have changed from string type to byte array type.
types.ValidateAccAddress(STRING) + types.AccAddress(STRING)→types.AccAddressFromBech32(STRING)
types.ValidateValAddress(STRING) + types.ValAddress(STRING)→types.ValAddressFromBech32(STRING)
types.ValidateConsAddress(STRING) + types.ConsAddress(STRING)→types.ConsAddressFromBech32(STRING)
types.AccAddress(STRING)→types.AccAddressFromBech32(STRING)
types.ValAddress(STRING)→types.ValAddressFromBech32(STRING)
types.ConsAddress(STRING)→types.ConsAddressFromBech32(STRING)
types.BytesToAccAddress(BYTES)→types.AccAddress(BYTES)
types.BytesToValAddress(BYTES)→types.ValAddress(BYTES)
types.BytesToConsAddress(BYTES)→types.ConsAddress(BYTES)

Additional changes:

Fix and improve lbm-sdk
- Fix invalid sumcheck address
  - https://github.com/line/lbm-sdk/pull/603/files#diff-ceb5513cfac0e7a49b0b5d44db5eea090c8d644d57946c64720aac8195363380L71-R73
- Fix the bug that should be used operator address when to is nil on CreateFTClass
  - https://github.com/line/lbm-sdk/pull/603/files#diff-8bde5886bff65fcafc1d529eec14b27ef07528c9d92fd2ce8469ffbb4a35c3eeL299-R437
- Fix incorrect address type
  - https://github.com/line/lbm-sdk/pull/603/files#diff-a124b23c2ecfa30a15bbae43eb33ca9552635de7c40286b0e693aa9278dd9fc2L252-R252
- Add address validation if address are used without validation.
Fix the following cosmos-sdk bug

closes: #578

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Checklist:

I followed the contributing guidelines and code of conduct.
I have added a relevant changelog to CHANGELOG.md
I have added tests to cover my changes.
I have updated the documentation accordingly.
I have updated API documentation client/docs/swagger-ui/swagger.yaml

…eateFTClass

Fix the bug that should be used `operator` address when `to` address is nil on CreateFTClass

codecov · 2022-07-21T08:53:58Z

Codecov Report

Merging #603 (88b2e28) into main (1bc3aac) will decrease coverage by 1.18%.
The diff coverage is 39.78%.

@@            Coverage Diff             @@
##             main     #603      +/-   ##
==========================================
- Coverage   60.60%   59.41%   -1.19%     
==========================================
  Files         814      814              
  Lines       94798    95390     +592     
==========================================
- Hits        57448    56680     -768     
- Misses      34097    35158    +1061     
- Partials     3253     3552     +299

Impacted Files	Coverage Δ
client/rpc/validators.go	`1.53% <0.00%> (ø)`
server/config/config.go	`41.35% <ø> (-0.44%)`	⬇️
server/init.go	`0.00% <0.00%> (ø)`
simapp/export.go	`12.30% <0.00%> (-0.50%)`	⬇️
simapp/simd/cmd/root.go	`81.81% <ø> (+0.96%)`	⬆️
simapp/test_helpers.go	`6.72% <0.00%> (+0.02%)`	⬆️
x/auth/client/cli/query.go	`2.15% <0.00%> (ø)`
x/auth/client/cli/validate_sigs.go	`0.00% <0.00%> (ø)`
x/auth/client/testutil/suite.go	`96.50% <0.00%> (+0.08%)`	⬆️
x/auth/client/tx.go	`36.66% <0.00%> (ø)`
... and 213 more

zemyblue · 2022-07-22T05:59:22Z

Please add this changes to CHANGELOG.md

tnasu

LGTM

tnasu · 2022-07-22T06:32:19Z

x/collection/keeper/msg_server.go

+
+	toAddr, err := sdk.AccAddressFromBech32(req.To)
+	if err != nil {
+		toAddr = operatorAddr


memo: Bugfix of lbm-sdk

tnasu · 2022-07-22T06:59:35Z

x/genutil/collect.go

 		if !valOk {
 			_, file, no, ok := runtime.Caller(1)
 			if ok {
-				fmt.Printf("CollectTxs-2, called from %s#%d - %s\n", file, no, sdk.BytesToAccAddress(valAddr).String())
+				fmt.Printf("CollectTxs-2, called from %s#%d - %s\n", file, no, valAddr.String())


memo: Bugfix of cosmos-sdk

tnasu · 2022-07-22T07:01:14Z

x/slashing/keeper/querier.go

-	signingInfo, found := k.GetValidatorSigningInfo(ctx, sdk.ConsAddress(params.ConsAddress))
+	// https://github.com/cosmos/cosmos-sdk/issues/12573
+	// Will be removed, but fix this
+	addr, err := sdk.ConsAddressFromBech32(params.ConsAddress)


memo: Bugfix of cosmos-sdk

tnasu · 2022-07-22T07:02:16Z

x/staking/legacy/v043/store_test.go

 	_, _, addr3 := testdata.KeyTestPubAddr()
-	consAddr := sdk.AccAddress(addr3).ToConsAddress()
+	consAddr := sdk.ConsAddress(addr3)


memo: Bugfix of cosmos-sdk

zemyblue

Please remove AddressDelimiter of staking and distribution module if we don't need anymore.

0Tech

Also, I have submitted a relevant PR to your repository.

0Tech · 2022-07-22T07:13:54Z

x/token/keeper/msg_server.go

 	if _, err := s.keeper.GetGrant(ctx, req.ContractId, granter, req.Permission); err != nil {
+


Suggested change

0Tech · 2022-07-22T07:18:01Z

x/token/keeper/msg_server.go

 	if _, err := s.keeper.GetGrant(ctx, req.ContractId, grantee, req.Permission); err != nil {
+


Suggested change

0Tech · 2022-07-22T08:07:07Z

x/stakingplus/authz.go

@@ -27,7 +27,7 @@ func (a CreateValidatorAuthorization) Accept(ctx sdk.Context, msg sdk.Msg) (foun
 }

 func (a CreateValidatorAuthorization) ValidateBasic() error {
-	if err := sdk.ValidateValAddress(a.ValidatorAddress); err != nil {
+	if _, err := sdk.AccAddressFromBech32(a.ValidatorAddress); err != nil {


Suggested change

if _, err := sdk.AccAddressFromBech32(a.ValidatorAddress); err != nil {

if _, err := sdk.ValAddressFromBech32(a.ValidatorAddress); err != nil {

Which means, the logic is not covered by unit tests. I should revisit x/stakingplus later and add the tests.

Will that tests be included in this PR?

No, it would be added on another PR.

OK, It might be better to submit an issue about it.

zemyblue · 2022-07-22T09:14:48Z

x/wasm/keeper/api.go

@@ -30,14 +28,14 @@ func (a cosmwasmAPIImpl) humanAddress(canon []byte) (string, uint64, error) {
 	gas := a.gasMultiplier.FromWasmVMGas(5)
 	if err := sdk.VerifyAddressFormat(canon); err != nil {
 		//nolint:stylecheck


This is very small thing. Does this nolint need?

It looks good to delete it.

zemyblue

I think AddressDenomDelimiter also isn't used. and AddressToPrefixKey function of bank module also.

Fix validation in the tests to avoid modifying test cases

0Tech

Approval, with some minor comments.

0Tech · 2022-07-24T06:35:51Z

x/wasm/keeper/handler_plugin_encoders_test.go

@@ -249,15 +249,15 @@ func TestEncoding(t *testing.T) {
 			srcMsg: wasmvmtypes.CosmosMsg{
 				Staking: &wasmvmtypes.StakingMsg{
 					Delegate: &wasmvmtypes.DelegateMsg{
-						Validator: sdk.BytesToAccAddress(valAddr).String(),
+						Validator: valAddr.String(),


The type of valAddr is sdk.ValAddress, so technically, it should be converted to sdk.AccAddress first.
However, from the background, it seems that it should have been sdk.ValAddress. Is it a bug-fix?

This is changed from CosmWasm/wasmd-v0.27.0. Yes, it's our bug fix.

https://github.com/CosmWasm/wasmd/blob/v0.27.0/x/wasm/keeper/handler_plugin_encoders_test.go#L251

Added this bug fix to description.

0Tech · 2022-07-24T07:03:22Z

x/wasm/keeper/legacy_querier.go

+		bz, err := keeper.QuerySmart(ctx, contractAddr, msg)
+		return bz, err


Suggested change

bz, err := keeper.QuerySmart(ctx, contractAddr, msg)

return bz, err

return keeper.QuerySmart(ctx, contractAddr, msg)

I would like to know the reason for this change. By the linter?

This is changed from CosmWasm/wasmd-v0.27.0. We should be the same since we continue to merge from CosmWasm/wasmd.

https://github.com/CosmWasm/wasmd/blob/v0.27.0/x/wasm/keeper/legacy_querier.go#L116

0Tech · 2022-07-24T07:11:29Z

x/wasm/keeper/legacy_querier_test.go

@@ -293,7 +291,7 @@ func TestLegacyQueryContractHistory(t *testing.T) {
 			var defaultQueryGasLimit sdk.Gas = 3000000
 			q := NewLegacyQuerier(keeper, defaultQueryGasLimit)
 			queryContractAddr := spec.srcQueryAddr
-			if queryContractAddr.Empty() {
+			if queryContractAddr == nil {


Suggested change

if queryContractAddr == nil {

if len(queryContractAddr) == 0 {

It will cover both of nil and an empty array. Is the comparison with nil intentional?

This is changed from CosmWasm/wasmd-v0.27.0. We should be the same since we continue to merge from CosmWasm/wasmd.

https://github.com/CosmWasm/wasmd/blob/v0.27.0/x/wasm/keeper/legacy_querier_test.go#L294

tnasu · 2022-07-25T02:34:17Z

I found a minor diff.

TypeURL: "/lbm.bank.invalidversion.MsgSend",

https://github.com/CosmWasm/wasmd/blob/v0.27.0/x/wasm/keeper/handler_plugin_encoders_test.go#L377

TypeURL: "/cosmos.bank.v2.MsgSend",

ulbqb added 2 commits July 21, 2022 13:35

Apply types/address.go from [email protected]

2bc8bb6

Fix syntax and semantic errors

83b42d6

ulbqb changed the title ~~Apply types/address.go from [email protected]~~ fix: apply types/address.go from [email protected] Jul 21, 2022

ulbqb mentioned this pull request Jul 21, 2022

[WIP] Catch up with address.go in cosmos-sdk v0.45.1 #587

Closed

5 tasks

ulbqb marked this pull request as draft July 21, 2022 05:43

ulbqb and others added 3 commits July 21, 2022 16:56

Delete unused variables

40fefdc

Fix the bug that should be used operator address when to is nil on Cr…

3fbba92

…eateFTClass

Merge pull request #16 from tnasu/fix/issue578_squash

2fe48d0

Fix the bug that should be used `operator` address when `to` address is nil on CreateFTClass

ulbqb requested a review from zemyblue July 22, 2022 02:19

ulbqb marked this pull request as ready for review July 22, 2022 02:22

0Tech self-requested a review July 22, 2022 02:39

test: check signers after validate basic (x/collection)

b4b6981

ulbqb self-assigned this Jul 22, 2022

Add this PR changes to CHANGELOG.md

99fd0d4

dudong2 self-requested a review July 22, 2022 07:14

tnasu reviewed Jul 22, 2022

View reviewed changes

0Tech added 2 commits July 22, 2022 07:27

test: check signers after validate basic (x/token)

11f3af3

test: check signers after validate basic (x/foundation)

846759e

zemyblue reviewed Jul 22, 2022

View reviewed changes

0Tech reviewed Jul 22, 2022

View reviewed changes

ulbqb added 3 commits July 22, 2022 17:42

Remove AddressDelimiter from x/staking and x/distribution

5d20854

Remove new line

da92dc4

Fix worng fix in x/stakingplus

6eb4123

zemyblue reviewed Jul 22, 2022

View reviewed changes

ulbqb added 2 commits July 22, 2022 18:28

Add a comment in x/collection

2b02c52

Remove meaningless nolint

11e8836

zemyblue reviewed Jul 22, 2022

View reviewed changes

Merge pull request #17 from 0Tech/fix/issue578_squash

670fa27

Fix validation in the tests to avoid modifying test cases

0Tech self-requested a review July 24, 2022 07:42

0Tech approved these changes Jul 24, 2022

View reviewed changes

Remove unused variable and function

88b2e28

ulbqb requested a review from zemyblue July 25, 2022 02:41

zemyblue approved these changes Jul 25, 2022

View reviewed changes

ulbqb merged commit 253d07e into Finschia:main Jul 25, 2022

dudong2 mentioned this pull request Sep 5, 2022

remove Bech32CacheSize #664

Closed

4 tasks

zemyblue mentioned this pull request Oct 27, 2022

docs: release 0.46.0-rc9 #765

Merged

5 tasks

zemyblue mentioned this pull request Nov 28, 2022

Release lbm-sdk v0.46.0 #812

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: apply types/address.go from [email protected] #603

fix: apply types/address.go from [email protected] #603

ulbqb commented Jul 21, 2022 •

edited

Loading

codecov bot commented Jul 21, 2022 •

edited

Loading

zemyblue commented Jul 22, 2022

tnasu left a comment

tnasu Jul 22, 2022

tnasu Jul 22, 2022

tnasu Jul 22, 2022

tnasu Jul 22, 2022

zemyblue left a comment

0Tech left a comment

0Tech Jul 22, 2022

0Tech Jul 22, 2022

0Tech Jul 22, 2022

0Tech Jul 22, 2022

ulbqb Jul 22, 2022

0Tech Jul 22, 2022

ulbqb Jul 23, 2022

zemyblue Jul 22, 2022

ulbqb Jul 22, 2022

zemyblue left a comment

0Tech left a comment •

edited

Loading

0Tech Jul 24, 2022

tnasu Jul 25, 2022

ulbqb Jul 25, 2022

0Tech Jul 24, 2022

tnasu Jul 25, 2022

0Tech Jul 24, 2022

tnasu Jul 25, 2022

tnasu commented Jul 25, 2022 •

edited

Loading

		if _, err := s.keeper.GetGrant(ctx, req.ContractId, granter, req.Permission); err != nil {

	if _, err := sdk.AccAddressFromBech32(a.ValidatorAddress); err != nil {
	if _, err := sdk.ValAddressFromBech32(a.ValidatorAddress); err != nil {

		bz, err := keeper.QuerySmart(ctx, contractAddr, msg)
		return bz, err

	bz, err := keeper.QuerySmart(ctx, contractAddr, msg)
	return bz, err
	return keeper.QuerySmart(ctx, contractAddr, msg)

	if queryContractAddr == nil {
	if len(queryContractAddr) == 0 {

fix: apply types/address.go from [email protected] #603

fix: apply types/address.go from [email protected] #603

Conversation

ulbqb commented Jul 21, 2022 • edited Loading

Description

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Checklist:

codecov bot commented Jul 21, 2022 • edited Loading

Codecov Report

zemyblue commented Jul 22, 2022

tnasu left a 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

zemyblue left a comment

Choose a reason for hiding this comment

0Tech left a 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

zemyblue left a comment

Choose a reason for hiding this comment

0Tech left a comment • 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

tnasu commented Jul 25, 2022 • edited Loading

ulbqb commented Jul 21, 2022 •

edited

Loading

codecov bot commented Jul 21, 2022 •

edited

Loading

0Tech left a comment •

edited

Loading

tnasu commented Jul 25, 2022 •

edited

Loading