LGTM alerts audit

[fedekunze]

Fixes issues reported by LGTM.com.

[fedekunze]

I wonder why the validator value is not set to store?

[fedekunze]

cc: @cwgoes @alexanderbez

[alexanderbez]

AddValidatorTokensAndShares stores the validator already via a call to k.SetValidator. This line can be _, newShares = k.AddValidatorTokensAndShares(ctx, validator, bondAmt).

[codecov]

Codecov Report

Merging #7440 into master will decrease coverage by 0.00%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master    #7440      +/-   ##
==========================================
- Coverage   55.10%   55.09%   -0.01%     
==========================================
  Files         588      588              
  Lines       36787    36782       -5     
==========================================
- Hits        20270    20266       -4     
+ Misses      14416    14415       -1     
  Partials     2101     2101              

[lgtm-com]

This pull request fixes 5 alerts when merging 45f11ae into 2c93ec7 - view on LGTM.com

fixed alerts:

• 3 for Incorrect conversion between integer types
• 1 for Useless assignment to local variable
• 1 for Useless assignment to field

[alessio]

The string "fields must not be negative. got %d" is very informative. What will it be replaced with?

[fedekunze]

we don't need it as the string is already cast to uint

[alessio]

@fedekunze dixit:

Don't know how to fix the keyring one tho.

Which line is it? I can't find the alert

[lgtm-com]

This pull request fixes 5 alerts when merging 10c3e33 into 2c93ec7 - view on LGTM.com

fixed alerts:

• 3 for Incorrect conversion between integer types
• 1 for Useless assignment to local variable
• 1 for Useless assignment to field

[cwgoes]

haha OK

[odeke-em]

Why did this code get removed?

[fedekunze]

not used apparently

[odeke-em]

Thank you @fedekunze for this change! However, I think LGTM gave you a false positive and I'd roll back this change.

It is most definitely depending on just single static assignment analyses of sorts, (so basically shaving off unnecessary operations) but really the parameters to deriveKey take in index. By the specification in https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki, whether or not harden == true, index has to be in the range 0 to 1<<31 - 1, but with this change, someone can now pass in a uint32 :( And by the spec under section https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#extended-keys

but even more subtle is that we were expecting always an int32, and if harden is set, we explicitly set the 1<<32 bit on as per the spec in

cosmos-sdk/crypto/hd/hdpath.go

Lines 209 to 216 in 2c93ec7

	func derivePrivateKey(privKeyBytes [32]byte, chainCode [32]byte, index uint32, harden bool) ([32]byte, [32]byte) {
	var data []byte
	if harden {
	index |= 0x80000000
	data = append([]byte{byte(0)}, privKeyBytes[:]...)
	} else {

so that won't necessarily change much if the value is already large

We shall definitely need a test for extended keys without harden set and ensure that we don't accept values greater than (1<<31-1)

Perhaps let me file an issue too.

[odeke-em]

Lol @LGTM.io, what happens if someone imports and then uses that code? It’s going to cause a bunch of undefined behaviour. For sets like that I’d personally ignore because the inverse won’t be caught by that service.

…

On Thu, Oct 22, 2020 at 6:02 AM Federico Kunze ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In x/simulation/mock_tendermint.go <#7440 (comment)>: > // validator already exists - mVal.val = update not used apparently — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#7440 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFL3V5EZWBS3TZE33F2ZYTSMAUPZANCNFSM4SBQF6BA> .