Gyro E-CLP #1055
Gyro E-CLP #1055
Conversation
Co-authored-by: Juan Ignacio Ubeira <[email protected]>
There was a problem hiding this comment.
First pass done. Great start really, looks mostly cooked!
I just have very minor comments. The only part I'd consider changing is the parameter validation, where the use of if (!original_condition) revert makes it kinda harder to read.
I think this is a good reason to compile this package with 0.8.27 and just use requires, preserving the original redability. Other than that the rest looks correct.
I understand the tests can be expanded to test more ECLP settings, but that's totally out of scope of this PR. Let's open a ticket to track that down anyways.
Awesome job!
There was a problem hiding this comment.
I understand the only change here is revert reasons right?
There was a problem hiding this comment.
Yes, I didn't want to change the math to 0.8 because we were close to auditing.
There was a problem hiding this comment.
Thanks @joaobrunoah, but I think we need to polish the param validation a little bit. I made just a few comments but the pattern is repeated across the math file (not sure if elsewhere yet).
Let's try to write those requires exactly as they were in the original code, simply replacing _grequire with the standard requre and the gyro error with a custom error, preserving the semantics. Note that in most cases <= and >= are used instead of strict operators.
|
I watched it the first time, I think I'll need to watch it a few more times. |
|
I have a small question. Why can't we take a ready math contract and reuse it? I assume that the problem is in dependencies, but it is still interesting to know WDYT |
You mean gyro math or signed math? |
I mean reusing GyroECLPMath from the original repo |
Description
This PR adapts Gyro E-CLP to balancer v3.
Type of change
Checklist:
main, or there's a description of how to mergeIssue Resolution
Closes #705