Skip to content

Comments

review of ramses double staking changes vs velo gauges#41

Open
imrtlfarm wants to merge 4 commits intov2-velo-gauge-audit-fixesfrom
v2-ramses-gauge
Open

review of ramses double staking changes vs velo gauges#41
imrtlfarm wants to merge 4 commits intov2-velo-gauge-audit-fixesfrom
v2-ramses-gauge

Conversation

@imrtlfarm
Copy link

No description provided.

Copy link
Author

@imrtlfarm imrtlfarm left a comment

Choose a reason for hiding this comment

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

main concern left is why we dont loop thru multi rewards on the velo side. do they not exist?

@@ -1,268 +1,119 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
pragma solidity =0.7.6 || ^0.8.13;
Copy link
Author

Choose a reason for hiding this comment

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

interesting pragma

}
// Deposit the LP in the gauge
IGauge(pool.gauge).deposit(balance);
IGauge(pool.gauge).deposit(balance, 0);
Copy link
Author

Choose a reason for hiding this comment

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

why is tokenid hardcoded to 0?

Copy link
Author

Choose a reason for hiding this comment

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

skips NFT-voter-gauge attachment part, makes sense


// claim rewards before disabling gauge
if (_claimRewards) {
IGauge(gauge).getReward(address(this));
Copy link
Author

Choose a reason for hiding this comment

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

How come we do not loop thru rewards in the velo version???

_rewardTokensArray[0] = _rewardTokens;
voter.claimRewards(gauges, _rewardTokensArray);

IERC20 rewardToken = IERC20(gauge.rewardToken());
Copy link
Author

Choose a reason for hiding this comment

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

same as above why no looping here?

test = 'test/foundry'
cache_path = 'forge-cache'
script = 'scripts'
evm_version = 'shanghai'
Copy link
Author

Choose a reason for hiding this comment

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

why this evm version?

Copy link
Author

Choose a reason for hiding this comment

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

this will brick us on non-push0 chains. Optimism and arbitrum have push0. superchains like mode and fraxtal should. Mantle and most other chains should also have this but it still prob doesn't make sense to set to shanghai

Copy link

Choose a reason for hiding this comment

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

ramses' deployed contract bytecode didnt have push0, throwing errors

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants