refactor(`participation`): use `sdk.Int` type for allocations #894

aljo242 · 2022-07-17T18:54:38Z

What does this PR does?

Removes a potential panic when calling SDK TruncateInt64 for the TotalAllocations() query. An error is now returned before the panic can be reached.

codecov · 2022-07-17T18:58:04Z

Codecov Report

Merging #894 (663509b) into develop (e185661) will decrease coverage by 0.00%.
The diff coverage is 13.46%.

@@             Coverage Diff             @@
##           develop     #894      +/-   ##
===========================================
- Coverage    10.56%   10.55%   -0.01%     
===========================================
  Files          327      327              
  Lines        75110    75191      +81     
===========================================
+ Hits          7936     7939       +3     
- Misses       66984    67062      +78     
  Partials       190      190

Impacted Files	Coverage Δ
...participation/types/auction_used_allocations.pb.go	`1.20% <0.00%> (-0.05%)`	⬇️
x/participation/types/events.pb.go	`0.86% <0.00%> (-0.03%)`	⬇️
x/participation/types/params.pb.go	`0.70% <0.00%> (-0.02%)`	⬇️
x/participation/types/query.pb.go	`0.67% <0.00%> (-0.01%)`	⬇️
x/participation/types/used_allocations.pb.go	`1.49% <0.00%> (-0.08%)`	⬇️
x/participation/keeper/msg_withdraw_allocations.go	`83.78% <50.00%> (ø)`
x/participation/keeper/available_allocations.go	`100.00% <100.00%> (ø)`
x/participation/keeper/grpc_used_allocations.go	`86.04% <100.00%> (ø)`
x/participation/keeper/msg_participate.go	`100.00% <100.00%> (ø)`
x/participation/keeper/total_allocations.go	`100.00% <100.00%> (ø)`
... and 10 more

lumtis

Great catch!

We actually have a test case that checks with negative delegation and I may think it should have triggered the panic and so triggering test failure.

Would it be possible to replace in total_allocation_test.go, wantError bool with a more accurate err error and assert ErrorIs(...) with ErrInvalidAddress and ErrInvalidAllocationAmount?
So we ensure the correct check if performed for negative value

tbruyelle · 2022-07-21T10:08:28Z

Hey sorry to stick my nose here, but I really don't understand how a panic can happen when calling TruncateInt64() on a negative number.

For instance sdk.NewDec(-1).TruncateInt64() doesn't trigger any panic and just returns -1.

Do you have the trace of the panic @aljo242 please ?

giunatale · 2022-07-21T10:15:46Z

Hey sorry to stick my nose here, but I really don't understand how a panic can happen when calling TruncateInt64() on a negative number.

For instance sdk.NewDec(-1).TruncateInt64() doesn't trigger any panic and just return -1.

Do you have the trace of the panic @aljo242 please ?

-1 is ok, but the panic risk do indeed exist. This is visible when looking at the source:
https://github.com/cosmos/cosmos-sdk/blob/main/math/dec.go#L676

An out of bound number would cause a panic. I would actually guess that the risk exist on both ends (so also a really big positive number)

tbruyelle · 2022-07-21T11:05:48Z

An out of bound number would cause a panic. I would actually guess that the risk exist on both ends (so also a really big positive number)

Indeed, so maybe you can also cover the big positive number case with an additional condition, wdyt ?

aljo242 · 2022-07-21T12:36:43Z

Yes @tbruyelle , I hadn't fully understood the cause of the panic when I wrote this - apologies. I ran into a panic with a large negative number during simulation testing. Modifying this PR now !

lumtis · 2022-07-21T12:39:32Z

Hey sorry to stick my nose here

This is very appreciated!

My bad, I misread the method as well looking at the PR.

My suggestion would be to refactor and remove completely the usage of uint64 and changing the logic of the module to use sdk.Int

tbruyelle · 2022-07-21T14:30:05Z

Question, why do we deal with uint64 in query response like

message QueryGetAvailableAllocationsResponse {
  uint64 availableAllocations = 1;
}

to represent something that can overlap int64 (I assume this was the reason of the panic). Why not a simple string ?
I noticed that strings are often used in the cosmos-sdk to represent numbers, it is to mitigate overlapping, right?

(both sdk.Dec and sdk.Int marshal into string)

giunatale · 2022-07-21T15:17:18Z

I noticed that strings are often used in the cosmos-sdk to represent numbers, it is to mitigate overlapping, right?

(both sdk.Dec and sdk.Int marshal into string)

It's because the protobuf encoding would be non-deterministic otherwise, that's the reason for using strings. Would be the same using bytes, I guess strings makes it easier also for reading the json equivalent.

Question, why do we deal with uint64 in query response

Allocations cannot be negative, proper typing that scopes out the result prevents errors. The panic was due to an out-of-bound error, the fact that it was a negative number was just coincidental. A big positive number would have caused the same issue

tbruyelle · 2022-07-21T15:41:36Z

Thanks for the explanation @giunatale 🙏

Just a though about replacing uint64 by sdk.Int in the return of GetTotalAllocations: it seems like the conversion from sdk.Dec to sdk.Int can still trigger a panic if the underlying big.Int is out of some specific bounds (see sdk.Dec.TruncateInt()). Not sure if this is the same bounds than in sdk.Dec.TruncateInt64 that said.

aljo242 · 2022-07-21T15:47:09Z

Thanks for the explanation @giunatale pray

Just a though about replacing uint64 by sdk.Int in the return of GetTotalAllocations: it seems like the conversion from sdk.Dec to sdk.Int can still trigger a panic if the underlying big.Int is out of some specific bounds (see sdk.Dec.TruncateInt()). Not sure if this is the same bounds than in sdk.Dec.TruncateInt64 that said.

Yes, it appears that the max bitlength for sdk.Int is 256 as opposed to 64, so we at least have larger bounds to operate within.

lumtis · 2022-07-21T19:30:14Z

Is this ready for review @aljo242 ?

aljo242 · 2022-07-21T20:14:59Z

Ready for review. Thanks for the @tbruyelle !

x/participation/keeper/msg_withdraw_allocations_test.go

proto/participation/auction_used_allocations.proto

lumtis

LGTM

What about also applying my first suggestion?

Would it be possible to replace in total_allocation_test.go, wantError bool with a more accurate err error and assert ErrorIs(...) with ErrInvalidAddress and ErrInvalidAllocationAmount?
So we ensure the correct check if performed for negative value

testutil/networksuite/networksuite.go

testutil/sample/participation.go

Co-authored-by: Thomas Bruyelle <thomas.bruyelle@gmail.com>

Co-authored-by: Lucas Btd <lucas.bertrand.22@gmail.com>

lumtis

We should fix genesis in localnet

The only changes should be to put requiredAllocations values in spn/genesis_template.json and testnet/genesis_template.json in quotes as those are now sdk.Int to be unmarshalled

localnet/spn/genesis_template.json

fix check

29e527c

aljo242 requested review from ilgooz and lumtis as code owners July 17, 2022 18:54

aljo242 self-assigned this Jul 17, 2022

aljo242 requested a review from giunatale as a code owner July 17, 2022 18:54

lumtis reviewed Jul 18, 2022

View reviewed changes

Merge branch 'develop' into fix/total-allocations-panic

2d55236

Merge branch 'develop' into fix/total-allocations-panic

70e90e4

aljo242 added 5 commits July 21, 2022 12:18

update proto

450fc6b

proto compile

836d413

refactor

04be9a2

format

4709b49

fix test state initialization

515da32

aljo242 changed the title ~~fix(participation): potential panic on keeper TotalAllocations function~~ refactor(participation): use sdk.Int type for allocations Jul 21, 2022

tbruyelle reviewed Jul 22, 2022

View reviewed changes

x/participation/keeper/msg_withdraw_allocations_test.go Outdated Show resolved Hide resolved

tbruyelle reviewed Jul 22, 2022

View reviewed changes

proto/participation/auction_used_allocations.proto Show resolved Hide resolved

lumtis reviewed Jul 22, 2022

View reviewed changes

testutil/networksuite/networksuite.go Outdated Show resolved Hide resolved

testutil/sample/participation.go Outdated Show resolved Hide resolved

Update x/participation/keeper/msg_withdraw_allocations_test.go

c8f8e55

Co-authored-by: Thomas Bruyelle <thomas.bruyelle@gmail.com>

Alex Johnson and others added 2 commits July 22, 2022 13:42

Update testutil/sample/participation.go

d204185

Co-authored-by: Lucas Btd <lucas.bertrand.22@gmail.com>

add sample.Int()

7199f22

aljo242 requested a review from lumtis July 22, 2022 17:59

Merge branch 'develop' into fix/total-allocations-panic

79e6103

lumtis suggested changes Jul 25, 2022

View reviewed changes

update requiredAllocations values

a3a8220

aljo242 requested a review from lumtis July 25, 2022 13:00

Merge branch 'develop' into fix/total-allocations-panic

bb30652

lumtis suggested changes Jul 25, 2022

View reviewed changes

localnet/spn/genesis_template.json Show resolved Hide resolved

tbruyelle previously approved these changes Jul 25, 2022

View reviewed changes

Merge branch 'develop' into fix/total-allocations-panic

473c812

aljo242 dismissed tbruyelle’s stale review via 473c812 July 26, 2022 16:51

aljo242 added 2 commits July 26, 2022 12:54

fix templates

52450d1

fix test

663509b

lumtis approved these changes Jul 26, 2022

View reviewed changes

lumtis merged commit f310279 into develop Jul 26, 2022

lumtis deleted the fix/total-allocations-panic branch July 26, 2022 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(`participation`): use `sdk.Int` type for allocations #894

refactor(`participation`): use `sdk.Int` type for allocations #894

aljo242 commented Jul 17, 2022

codecov bot commented Jul 17, 2022 •

edited

Loading

lumtis left a comment

tbruyelle commented Jul 21, 2022 •

edited

Loading

giunatale commented Jul 21, 2022 •

edited

Loading

tbruyelle commented Jul 21, 2022

aljo242 commented Jul 21, 2022

lumtis commented Jul 21, 2022

tbruyelle commented Jul 21, 2022 •

edited

Loading

giunatale commented Jul 21, 2022 •

edited

Loading

tbruyelle commented Jul 21, 2022 •

edited

Loading

aljo242 commented Jul 21, 2022

lumtis commented Jul 21, 2022

aljo242 commented Jul 21, 2022 •

edited

Loading

lumtis left a comment

lumtis left a comment

refactor(participation): use sdk.Int type for allocations #894

refactor(participation): use sdk.Int type for allocations #894

Conversation

aljo242 commented Jul 17, 2022

What does this PR does?

codecov bot commented Jul 17, 2022 • edited Loading

Codecov Report

lumtis left a comment

Choose a reason for hiding this comment

tbruyelle commented Jul 21, 2022 • edited Loading

giunatale commented Jul 21, 2022 • edited Loading

tbruyelle commented Jul 21, 2022

aljo242 commented Jul 21, 2022

lumtis commented Jul 21, 2022

tbruyelle commented Jul 21, 2022 • edited Loading

giunatale commented Jul 21, 2022 • edited Loading

tbruyelle commented Jul 21, 2022 • edited Loading

aljo242 commented Jul 21, 2022

lumtis commented Jul 21, 2022

aljo242 commented Jul 21, 2022 • edited Loading

lumtis left a comment

Choose a reason for hiding this comment

lumtis left a comment

Choose a reason for hiding this comment

refactor(`participation`): use `sdk.Int` type for allocations #894

refactor(`participation`): use `sdk.Int` type for allocations #894

codecov bot commented Jul 17, 2022 •

edited

Loading

tbruyelle commented Jul 21, 2022 •

edited

Loading

giunatale commented Jul 21, 2022 •

edited

Loading

tbruyelle commented Jul 21, 2022 •

edited

Loading

giunatale commented Jul 21, 2022 •

edited

Loading

tbruyelle commented Jul 21, 2022 •

edited

Loading

aljo242 commented Jul 21, 2022 •

edited

Loading