Inconsistent use of return and input arguments in (pool) redeemAuto()/redeemAuto2() and (tco2) retire()

[danceratopz]

This is a minor issue that impacts developers working directly with the Toucan core contracts.

1. The pool functions (e.g. in NCT.sol) redeemAuto() and redeemAuto2() return arrays of TCO2 addresses and corresponding amounts that were redeemed in exchange for the pool token. These arrays contain entries corresponding to the TCO2s with the lowest scores from scoredTCO2s. In particular, if the lowest scored TCO2 is not present in the NCT pool (i.e., the pool has balance 0) then these functions still return the TCO2 address with a corresponding entry of 0 in the amounts array.
2. The TCO2 retire() function (from ToucanCarbonOffsets.sol) takes a uint256 specifying the amount to retire. If this value is zero the transaction gets reverted, as _retire() then calls registerEvent()from RetirementCertifiates.sol which understandably requires that amount to be non-zero, see:
   

   contracts/contracts/RetirementCertificates.sol

   Line 137 in cf3d9e5

   amount != 0 && amount >= minValidRetirementAmount,

   .

A user would intuitively expect to be able to loop over the outputs of redeemAuto() and provide them to retire() as inputs, which is currently the case in the OffsetHelper, see example-implementations/blob/d5e6...94cf8/contracts/OffsetHelper.sol#L494.

Perhaps it would be better to remove TCO2 addresses with corresponding amount 0 from the output of the redeemAuto functions?

The background that led to this observation is described in ToucanProtocol/example-implementations#31.

[mauricedesaxe]

Aside from the fact that redeemAuto() in fact doesn't return any arrays, this is a very well-documented issue.

Perhaps it would be better to remove TCO2 addresses with corresponding amount 0 from the output of the autoRedeem functions?

I think this is the way to go. Fixing it in the OffsetHelper feels like patchwork to me. Fixing the root problem here is better.

I'll take this internally and see if the guys have any objections to such a check. Otherwise, we should be able to upgrade the core pool contracts soon.

[mauricedesaxe]

I'm literally just realizing it's super hard to work around Solidity's intricacies (can't really delete an item from an array, can't create dynamic arrays, etc)

[danceratopz]

Just fixed the redeemAuto* names in the ticket's title and description (autoRedeem -> redeemAuto).

[mauricedesaxe]

Just wanted to update this and say that we have merged a fix in the core contracts so they do not return TCO2s with 0 amounts anymore. The contracts are still to be upgraded, but this should be fixed when that happens. Let's keep your PR open as a draft until then.

[0xmichalis]

@danceratopz this is finally fixed for NCT in Polygon Mainnet and soon we will release the fix for BCT too. I am going to close this issue but feel free to reopen if it persists. Thanks!

[danceratopz]

Great, thanks for heads up @0xmichalis!

[0xmichalis]

This is fixed now also in BCT cc @cujowolf