dss-proxy-actions icon indicating copy to clipboard operation
dss-proxy-actions copied to clipboard

Upgrade to solc 0.6.12 and several improvements

Open gbalabasquer opened this issue 2 years ago • 3 comments

gbalabasquer avatar Aug 24 '22 15:08 gbalabasquer

So this is the new version of proxy actions. Changes done:

  • Upgrade to solc 0.6.12. Haven't done 0.8.x as it requires dependencies for the tests that won't run in that version of solc. We should evaluate if we want to change how tests work so we can use a newer solc version.

  • A lot of variables are now immutable and not passed via params anymore, you might think some of them could be a bit controversial (except the vat we all agree is fine), but all the changed ones so far have never been replaced by governance (daiJoin/dai, jug, pot, registry and manager). In the case of the end, history has shown that it is an address that can be upgraded, we might still want to set it as immutable and worst case redeploy. It would only affect DssProxyActionsEnd. An important caveat is these changes provoked a simplification (but still a change) of the APIs. Meaning that integrators that want to upgrade to new proxy actions will need to adapt the calls.

  • Other minor changes like using _divup directly or adding some missing overflow checks or standardizing every int/uint variable to use the 256 sufix.

From known issues I still have pending to define what to do in the following case:

  • When a user wants to draw DAI, they will land in _getDrawDart. That functions first checks if there is some vat.dai(urn) and discounts that number from the amount of DAI to be generated. This is needed as due dust or any other reason the user might have DAI there, so we do not generate extra. However there is a problem that at least once happened to a user (probably happened more times that were not identified). If the user is minting at dust value (or close to it) and there was some dust in the urn, then the system will try to generate below the dust, provoking a revert. Here the link of the case that was reported and the its discussion: https://discord.com/channels/893112320329396265/897479589171986434/955905952870203502 I'm wondering if we should do something about it at proxy actions level or UIs should try to solve for this situation.

gbalabasquer avatar Aug 25 '22 13:08 gbalabasquer

@talbaneth whenever you can give a final approval including the latest changes would be nice. We would send the code as it is for auditing and if the dust issue ends up changing something else, it will be reviewed separately

gbalabasquer avatar Nov 01 '22 15:11 gbalabasquer

@iamchrissmith @talbaneth will require a check in the last commit from you, thanks!

gbalabasquer avatar Dec 06 '22 12:12 gbalabasquer