-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
chore: adding deployment scripts #34
Conversation
kupermind
commented
Oct 23, 2024
- Adding deployment scripts and tests.
/// @param _manager Manager address. | ||
function initialize(address _manager) external{ | ||
function initialize() external{ | ||
// Check for already initialized | ||
if (owner != address(0)) { | ||
revert AlreadyInitialized(); | ||
} | ||
|
||
// Check for zero address | ||
if (_manager == address(0)) { | ||
revert ZeroAddress(); | ||
} | ||
|
||
owner = msg.sender; | ||
manager = _manager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cyclic dependency, need to deploy blanc and change the manager later.
@@ -50,7 +54,7 @@ error ServiceOwnerOnly(uint256 serviceId, address sender, address serviceOwner); | |||
/// @author Andrey Lebedev - <[email protected]> | |||
/// @author Tatiana Priemova - <[email protected]> | |||
/// @author David Vilela - <[email protected]> | |||
contract ContributeManager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contract must be ERC721TokenReceiver
- compatible
@@ -184,7 +190,7 @@ contract ContributeManager { | |||
IContributors(contributorsProxy).setServiceInfoForId(msg.sender, socialId, serviceId, multisig, stakingInstance); | |||
|
|||
// Approve service NFT for the staking instance | |||
IToken(serviceRegistry).approve(stakingInstance, serviceId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to correct ABI
@@ -195,13 +201,19 @@ contract ContributeManager { | |||
/// @param socialId Contributor social Id. | |||
/// @param stakingInstance Contribute staking instance address. | |||
function createAndStake(uint256 socialId, address stakingInstance) external payable { | |||
// Reentrancy guard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added reentrancy guards all the way through due to external function calls, even though we trust them
@@ -225,7 +237,8 @@ contract ContributeManager { | |||
uint256 threshold = IStaking(stakingInstance).threshold(); | |||
// Check for number of agent instances that must be equal to one, | |||
// since msg.sender is the only service multisig owner | |||
if (numAgentInstances != NUM_AGENT_INSTANCES || threshold != THRESHOLD) { | |||
if ((numAgentInstances > 0 && numAgentInstances != NUM_AGENT_INSTANCES) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If any of the fields are zeros in the staking contract, we need to ignore the check.
@@ -12,8 +12,8 @@ require("@nomicfoundation/hardhat-toolbox"); | |||
|
|||
const ALCHEMY_API_KEY_MAINNET = process.env.ALCHEMY_API_KEY_MAINNET; | |||
const ALCHEMY_API_KEY_MATIC = process.env.ALCHEMY_API_KEY_MATIC; | |||
const ALCHEMY_API_KEY_GOERLI = process.env.ALCHEMY_API_KEY_GOERLI; | |||
const ALCHEMY_API_KEY_MUMBAI = process.env.ALCHEMY_API_KEY_MUMBAI; | |||
const ALCHEMY_API_KEY_SEPOLIA = process.env.ALCHEMY_API_KEY_SEPOLIA; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing the hardhat config and adding more chains
@@ -0,0 +1,75 @@ | |||
/*global process*/ | |||
|
|||
const { ethers } = require("hardhat"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deployment is tested
@@ -0,0 +1 @@ | |||
{"contractVerification":true,"useLedger":true,"derivationPath":"m/44'/60'/2'/0/0","providerName":"base","networkURL":"https://mainnet.base.org","gasPriceInGwei":"2","bridgeMediatorAddress":"0xE49CB081e8d96920C38aA7AB90cb0294ab4Bc8EA","gnosisSafeAddress":"0x69f4D1788e39c87893C980c06EdF4b7f686e2938","gnosisSafeProxyFactoryAddress":"0xC22834581EbC8527d974F8a1c97E1bEA4EF910BC","fallbackHandlerAddress":"0x017062a1dE2FE6b99BE3d9d37841FeD19F573804","olasAddress":"0x54330d28ca3357F294334BDC454a032e7f353416","multisigProxyHash130":"0xb89c1b3bdf2cf8827818646bce9a8f6e372885f8c55e5c07acbd307cb133b000","serviceRegistryAddress":"0x3C1fF68f5aa342D296d4DEe4Bb1cACCA912D95fE","serviceRegistryTokenUtilityAddress":"0x34C895f302D0b5cf52ec0Edd3945321EB0f83dd5","serviceManagerTokenAddress":"0x63e66d7ad413C01A7b49C7FF4e3Bb765C4E4bd1b","gnosisSafeMultisigImplementationAddress":"0xBb7e1D6Cb6F243D6bdE81CE92a9f2aFF7Fbe7eac","stakingTokenAddress":"0xEB5638eefE289691EcE01943f768EDBF96258a80","stakingNativeTokenAddress":"","stakingFactoryAddress":"0x1cEe30D08943EB58EFF84DD1AB44a6ee6FEff63a","agentId":"6","configHash":"0xb89c1b3bdf2cf8827818646bce9a8f6e372885f8c55e5c07acbd307cb133b000","livenessRatio":"700000000000000","contributeActivityCheckerAddress":"","stakingParams":{"metadataHash":"","maxNumServices":"100","rewardsPerSecond":"1000000000000000","minStakingDeposit":"50000000000000000000","minNumStakingPeriods":"3","maxNumInactivityPeriods":"3","livenessPeriod":"86400","timeForEmissions":"2592000","numAgentInstances":"1","agentIds":["14"],"threshold":"0","configHash":"0x0000000000000000000000000000000000000000000000000000000000000000","proxyHash":"0xb89c1b3bdf2cf8827818646bce9a8f6e372885f8c55e5c07acbd307cb133b000","serviceRegistry":"0x3C1fF68f5aa342D296d4DEe4Bb1cACCA912D95fE","activityChecker":""},"contributeAgentAddress":""} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are they copied from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are copied from autonolas-governance
and autonolas-registries
docs/configuration.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Safes are from:
https://github.com/safe-global/safe-deployments/blob/main/src/assets/v1.3.0/gnosis_safe.json
https://github.com/safe-global/safe-deployments/blob/main/src/assets/v1.3.0/proxy_factory.json
https://github.com/safe-global/safe-deployments/blob/main/src/assets/v1.3.0/compatibility_fallback_handler.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The staking configuration is yet to be defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@77ph please confirm they're 100% copies
@@ -71,6 +71,26 @@ The audit is provided as development matures. The latest audit report can be fou | |||
- Create staking proxy instance on [Launch](https://launch.olas.network/); | |||
- Vote for staking contracts on [Govern](https://govern.olas.network/). | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checkpoint call arrow should come from ContributeService
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Checkpoint can be called by anyone. Well, technically the checkpoint is called also during stake()
, unstake()
and claim()
, but that's not connected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we intend it to be called by ContributeService and it doesn't make sense to introduce a third role. We certainly don't intend the user to call it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current graphic is correct but its not helpful when interfacing with Contribute team
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so let's remove this link or assign it to the Contribute service
? somebody needs to call checkpoint()
from time to time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assign to ContributeService
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ot just use AnyAccount
block