Fix autoRetire when the lowest scored tco2 is not present in the pool #31 #32
Conversation
This test case first creates the state described in the ticket as a setup step, ie, that the lowest scored TCO2 is not present in the NCT pool. Then it tests that autoRetire works, even if the lowest scored TCO2 is not in the pool.
Previously, a github action was triggered when pushing to a branch, but it the action ran with main.
|
Let's re-run these tests and see |
|
Turning this into a draft until we figure out if we can fix this in the core contracts. Awesome work @danceratopz, you've been a MAJOR help here 🙏🏻 |
|
|
||
| IToucanCarbonOffsets(_tco2s[i]).retire(_amounts[i]); | ||
|
|
||
| if (_amounts[i] > 0) { |
There was a problem hiding this comment.
We have a fix for this deployed in Mumbai for both BCT and NCT, you don't need this change anymore. Btw @danceratopz it would be great if you could confirm that the issue is fixed for you now!
There was a problem hiding this comment.
Hi @Kargakis, it's not possible to verify whether the issue has been fixed on Polygon Mainnet with the current chain state as there are plenty of the lowest scored TCO2, TCO2-VCS-1052-2012, in the NCT Pool, https://polygonscan.com/token/0x463de2a5c6e8bb0c87f4aa80a02689e6680f72c7#balances.
As the issue only reveals itself when the lowest scored TCO2 is not present in the pool, it's probably not possible to test this issue on Mumbai either, but I didn't check (I was a bit lazy as the ABI for NCT on Mumbai has not been published https://mumbai.polygonscan.com/address/0x7beCBA11618Ca63Ead5605DE235f6dD3b25c530E#readContract).
Btw, I don't think there are liquidity pools for NCT or BCT setup on Sushiswap on Mumbai, so we never extensively used or tested the OffsetHelper there, cf https://discord.com/channels/706114892666765375/929319519430799360/1014126215877103647
There was a problem hiding this comment.
@danceratopz good point on liquidity pool setup in Mumbai, @lazaralex98 this is something we need to have in place in order for the offsetter contract to be prod-ready, do we track TODOs for that anywhere atm?
The first TCO2 in the score array in BCT is empty, we can test this issue by redeeming BCT: https://mumbai.polygonscan.com/address/0xf2438A14f668b1bbA53408346288f3d7C71c10a1#readProxyContract
The offsetter contract btw should expose all the functions needed to offset without the need to swap so the lack of LPs in Mumbai should not block testing of this issue.
There was a problem hiding this comment.
@Kargakis Sorry, Michalis, I didn't check BCT on Mumbai. Yes, I can confirm the fix, the (original) Mumbai OffsetHelper now takes the second TCO2 in the list of scored TCO2s:
https://mumbai.polygonscan.com/tx/0xfbe23052dabc36e070d2b2b313ac9883159d3c6f2616c9ba21d0df91f6986cfb
There was a problem hiding this comment.
I was a bit lazy as the ABI for NCT on Mumbai has not been published
It has been published, but it's an upgradeable contract so you have to look at read as proxy.
I don't think there are liquidity pools for NCT or BCT setup on Sushiswap on Mumbai
We indeed do not have that, nor is this planned anywhere.
There was a problem hiding this comment.
good point on liquidity pool setup in Mumbai, @lazaralex98 this is something we need to have in place in order for the offsetter contract to be prod-ready, do we track TODOs for that anywhere atm?
We do have a Notion proposal with steps documented to take the OffsetHelper production ready. I'll add bringing LPs on Mumbai as a step & share it with you on Slack.
|
@lazaralex98, @Kargakis my initial fix had a classic gotcha (I forgot to increment the loop counter 🙈) so I pushed eb19d3a. None of the tests in test//OffsetHelper.ts caught this, I wondered if |
It might. I know for a fact it results in useless error logging and the reality is that the test would have failed (if it was supposed to) regardless of using Might make sense for this to be a step in our preparation of the OffsetHelper for production |
Adds a test case and a fix for #31.