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

[aljo242]

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[bot]]

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%)	:arrow_down:
x/participation/types/events.pb.go	0.86% <0.00%> (-0.03%)	:arrow_down:
x/participation/types/params.pb.go	0.70% <0.00%> (-0.02%)	:arrow_down:
x/participation/types/query.pb.go	0.67% <0.00%> (-0.01%)	:arrow_down:
x/participation/types/used_allocations.pb.go	1.49% <0.00%> (-0.08%)	:arrow_down:
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

[tbruyelle]

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]

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]

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]

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]

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]

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]

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]

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.

[aljo242]

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]

Is this ready for review @aljo242 ?

[aljo242]

Ready for review. Thanks for the @tbruyelle !