search

threshold-network / merkle-distribution

Threshold Network rewards generation and distribution
https://threshold.network
1 stars 6 forks source link

Add several batch claims functions and corresponding tests #5

Closed theref closed 2 years ago

theref commented 2 years ago

We want to be able to claim batches all together. This PR adds those in. There were two methods, one using 2D arrays to pass in the arguments, the other uses structs. The gas usage of those is:

·---------------------------------------------|----------------------------|-------------|-----------------------------·
|             Solc version: 0.8.9             ·  Optimizer enabled: false  ·  Runs: 200  ·  Block limit: 30000000 gas  │
··············································|····························|·············|······························
|  Methods                                                                                                             │
·························|····················|·············|··············|·············|···············|··············
|  Contract              ·  Method            ·  Min        ·  Max         ·  Avg        ·  # calls      ·  eur (avg)  │
·························|····················|·············|··············|·············|···············|··············
|  CumulativeMerkleDrop  ·  batchClaimArray   ·      25838  ·      317687  ·      83998  ·           10  ·          -  │
·························|····················|·············|··············|·············|···············|··············
|  CumulativeMerkleDrop  ·  batchClaimStruct  ·      22551  ·      318843  ·      81597  ·           10  ·          -  │
·························|····················|·············|··············|·············|···············|··············
|  CumulativeMerkleDrop  ·  claim             ·      53610  ·       89162  ·      84233  ·           91  ·          -  │
·························|····················|·············|··············|·············|···············|··············
|  CumulativeMerkleDrop  ·  setMerkleRoot     ·      30762  ·       47886  ·      41346  ·           34  ·          -  │
·························|····················|·············|··············|·············|···············|··············
|  TokenMock             ·  mint              ·      54235  ·       71311  ·      69997  ·           13  ·          -  │
·························|····················|·············|··············|·············|···············|··············
|  Deployments                                ·                                          ·  % of limit   ·             │
··············································|·············|··············|·············|···············|··············
|  CumulativeMerkleDrop                       ·          -  ·           -  ·    1668513  ·        5.6 %  ·          -  │
··············································|·············|··············|·············|···············|··············
|  TokenMock                                  ·          -  ·           -  ·    2411932  ·          8 %  ·          -  │
·---------------------------------------------|-------------|--------------|-------------|---------------|-------------·

Closes #2

vzotova commented 2 years ago

I'd leave only one method, struct looks wonderful. To compare methods even better we can measure size of contract with only struct method and only array method.

theref commented 2 years ago

There's something I don't understand in the batch claiming tests. Also, wdyt about adding tests for weird batch claims like claming for the same account multiple times in the same batch or stuff lie that?

Yeah I can add that. Is the expected behaviour that this would just revert in the claim function because there is nothing left to claim? or do you think there should be an extra require in batchClaim? (which would obviously add overhead for 99% of it's use)