Skip to content

Conversation

@duncan4123
Copy link

No description provided.

duncan4123 and others added 30 commits January 7, 2023 13:35
This reverts commit 2b20bd48f3c3d9be7fb81c71e8ea4946cac33241.
@@ -0,0 +1,17 @@
"use strict";
Copy link

Choose a reason for hiding this comment

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

@duncan4123 rename file to flowGovernor.js

Copy link
Author

Choose a reason for hiding this comment

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

this gets removed

Copy link

Choose a reason for hiding this comment

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

ok then remove it from this pull request as well?

const { deploy } = deployments;
const { deployer } = await getNamedAccounts();
const flow = await deployments.get('Flow');
await deploy('MerkleClaim', {
Copy link

Choose a reason for hiding this comment

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

@duncan4123 I thought you weren't going to do a merkle drop

Copy link
Author

Choose a reason for hiding this comment

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

yep merkle gets removed

Copy link

Choose a reason for hiding this comment

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

then remove the deployment script as well?

@@ -0,0 +1,32 @@
import { HardhatRuntimeEnvironment } from 'hardhat/types'
Copy link

Choose a reason for hiding this comment

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

@duncan4123 what is the removed folder about?

Copy link

Choose a reason for hiding this comment

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

@duncan4123 just yeet the whole folder?

//
console.log('Arbitrum Goerli Velocimeter Instruments deployed')

return true
Copy link

Choose a reason for hiding this comment

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

should delete all the code from return true to the end of file


const { deployer } = await getNamedAccounts()

// const escrow = await deployments.get('VotingEscrow')
Copy link

Choose a reason for hiding this comment

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

@duncan4123 generally speaking all the commented code should be deleted

@@ -0,0 +1 @@
42161
Copy link

Choose a reason for hiding this comment

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

@duncan4123 can probably remove the whole arbitrumOneOLD folder


escrow.setVoter(address(voter));

pairfactory.setVoter(address(voter));
Copy link

Choose a reason for hiding this comment

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

@duncan4123 are you supposed to test something with this?

Copy link
Author

Choose a reason for hiding this comment

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

if voter is not set then we cant set externalBribe on pair as voter is required to be the caller


// Accrue fees on token0
function _update0(uint256 amount) internal {
if (hasGauge == false) {
Copy link

Choose a reason for hiding this comment

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

@duncan4123 you can just say if (!hasGauge) for false and if (hasGauge) for true

Copy link

Choose a reason for hiding this comment

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

or if (hasGauge) { ... } else { ... }

IBribe(externalBribe).notifyRewardAmount(token0, amount); //transfer fees to exBribes
uint256 _ratio = (amount * 1e18) / totalSupply; // 1e18 adjustment is removed during claim
if (_ratio > 0) {
index0 += _ratio;
Copy link

Choose a reason for hiding this comment

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

@duncan4123 do we still need this index update?

Copy link
Author

Choose a reason for hiding this comment

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

It will be keeping track of something that we are not using. This is the part we need to be weary of. Could it cause any unintended consequences?

emit Fees(msg.sender, amount, 0);
}
if (hasGauge == true) {
IBribe(externalBribe).notifyRewardAmount(token0, amount); //transfer fees to exBribes
Copy link

Choose a reason for hiding this comment

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

@duncan4123 does ExternalBribe need to have ERC20 allowance from the Pair?

Copy link
Author

Choose a reason for hiding this comment

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

Yes it does. This is in the latest PR.

uint256 _ratio = amount * 1e18 / totalSupply; // 1e18 adjustment is removed during claim
if (_ratio > 0) {
index0 += _ratio;
// function _update0(uint256 amount) internal {
Copy link

Choose a reason for hiding this comment

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

@duncan4123 always remove unused code

// This makes tank updateable forever by the team address (multisig)

function acceptTank() external {
require(msg.sender == team, "not pending team");
Copy link

Choose a reason for hiding this comment

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

@duncan4123 shouldn't this be pendingTank instead of team

Copy link
Author

Choose a reason for hiding this comment

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

_updateFor(_gauge);
pools.push(_pool);
emit GaugeCreated(_gauge, msg.sender, _internal_bribe, _external_bribe, _pool);
Pair(_pool).setHasGauge(true); // may need to switch to IPair?
Copy link

Choose a reason for hiding this comment

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

@duncan4123 yeah you should use IPair if possible

Copy link
Author

Choose a reason for hiding this comment

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

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.

5 participants