Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Carbon Vortex 2.0 #140

Merged
merged 13 commits into from
Apr 19, 2024
Merged

Carbon Vortex 2.0 #140

merged 13 commits into from
Apr 19, 2024

Conversation

ivanzhelyazkov
Copy link
Collaborator

  • Add Carbon Vortex 2.0 contract
  • Add Carbon Vortex 2.0 tests

Based on this governance proposal:
https://gov.bancor.network/t/carbon-vortex-2-0-dutch-auction/4597

@ivanzhelyazkov ivanzhelyazkov requested a review from barakman April 10, 2024 08:25
@ashachaf ashachaf mentioned this pull request Apr 10, 2024
Open
barakman
barakman requested changes Apr 10, 2024
View reviewed changes
Copy link
Collaborator

@barakman barakman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: uint32(block.timestamp) is an unsafe cast which appears in about 5 different places in the code, but I suppose (know) that it is harmless in the foreseeable future...

contracts/utility/ExpDecayMath.sol Outdated Show resolved Hide resolved
contracts/vortex/interfaces/ICarbonVortex.sol Outdated Show resolved Hide resolved
contracts/vortex/interfaces/ICarbonVortex.sol Outdated Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Show resolved Hide resolved
test/math/ExpDecayMath.ts Outdated Show resolved Hide resolved
test/math/ExpDecayMath.ts Show resolved Hide resolved
test/forge/VortexTestCaseParser.t.sol Outdated Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Show resolved Hide resolved
barakman
barakman requested changes Apr 10, 2024
View reviewed changes
contracts/vortex/CarbonVortex.sol Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Outdated Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Show resolved Hide resolved
barakman
barakman requested changes Apr 10, 2024
View reviewed changes
contracts/vortex/CarbonVortex.sol Outdated Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Outdated Show resolved Hide resolved
barakman
barakman reviewed Apr 10, 2024
View reviewed changes
Copy link
Collaborator

@barakman barakman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably use extend-function-calc-exp-decay-input-range as the base branch here (and perhaps also pull from it or rebase on top of it), so that the diffs related to that branch don't show up on this PR.

@ivanzhelyazkov ivanzhelyazkov requested a review from barakman April 11, 2024 10:16
barakman
barakman requested changes Apr 12, 2024
View reviewed changes
Copy link
Collaborator

@barakman barakman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a few semantic comments, and a couple of important ones related to the "uint128 stuff", so I'd like to emphasize once again:
I think that we should have as little as possible "uint128 stuff" in the code, meaning that everything stays as uint256 right up to the point when we need to store it somewhere.

contracts/vortex/CarbonVortex.sol Outdated Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Outdated Show resolved Hide resolved
contracts/vortex/interfaces/ICarbonVortex.sol Outdated Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Outdated Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Outdated Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Show resolved Hide resolved
lbeder
lbeder requested changes Apr 15, 2024
View reviewed changes
Copy link
Collaborator

@lbeder lbeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a preliminary review, but FYI

contracts/helpers/TestVault.sol Show resolved Hide resolved
hardhat.config.ts Outdated Show resolved Hide resolved
test/math/ExpDecayMath.ts Show resolved Hide resolved
contracts/vortex/interfaces/ICarbonVortex.sol Outdated Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Outdated Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Outdated Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Outdated Show resolved Hide resolved
contracts/vortex/CarbonVortex.sol Outdated Show resolved Hide resolved
@ivanzhelyazkov ivanzhelyazkov requested a review from lbeder April 17, 2024 15:21
yudilevi
yudilevi requested changes Apr 18, 2024
View reviewed changes
contracts/vortex/CarbonVortex.sol Outdated Show resolved Hide resolved
@ivanzhelyazkov ivanzhelyazkov requested a review from yudilevi April 18, 2024 13:03
@ivanzhelyazkov ivanzhelyazkov merged commit c87e1bf into dev Apr 19, 2024
3 checks passed
@ivanzhelyazkov ivanzhelyazkov deleted the carbon-vortex-2.0 branch April 19, 2024 08:22
0xMotto pushed a commit to Velocimeter/carbon-contracts that referenced this pull request Oct 11, 2024
@ivanzhelyazkov
Merge pull request bancorprotocol#140 from bancorprotocol/carbon-vort…
e41f16a 
…ex-2.0

Carbon Vortex 2.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barakman barakman barakman approved these changes

@lbeder lbeder lbeder approved these changes

@yudilevi yudilevi yudilevi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ivanzhelyazkov @yudilevi @lbeder @barakman