refactor: mech manager to control marketplaces and factories #51
Conversation
kupermind
commented
Dec 13, 2024
•
kupermind
commented
- Mech manager to control marketplaces and factories;
- Payment handling for request deliveries;
- Everything is able to be processed by mech marketplaces only.
contracts/MechManager.sol
Outdated
| /// @dev Sets mech marketplace statues. | ||
| /// @param mechMarketplaces Mech marketplace contract addresses. | ||
| /// @param statuses Corresponding whitelisting statues. | ||
| function setMechMarketplaceStatuses(address[] memory mechMarketplaces, bool[] memory statuses) external { |
There was a problem hiding this comment.
Why do we have multiple marketplaces? That's really not needed and just adding confusion for a user of the system
| @@ -58,19 +57,25 @@ contract MechMarketplace is IErrorsMarketplace { | |||
| // Maximum response time | |||
| uint256 public immutable maxResponseTimeout; | |||
| // Mech karma contract address | |||
| address public immutable karmaProxy; | |||
There was a problem hiding this comment.
why changed the name?
There was a problem hiding this comment.
Cleaner - nobody cares in the contract if it's proxy or not, the setup matters
contracts/MechMarketplace.sol
Outdated
| // Number of undelivered requests | ||
| uint256 public numUndeliveredRequests; | ||
| // Number of total requests | ||
| uint256 public numTotalRequests; | ||
| // Reentrancy lock | ||
| uint256 internal _locked = 1; | ||
|
|
||
| // Map of request payments | ||
| mapping(uint256 => uint256) public mapRequestIdPayments; |
There was a problem hiding this comment.
Why do we need this? Can we not get away with a fee counter per mech?
There was a problem hiding this comment.
I don't understand what is meant by that. Funds don't go to mechs before requests are delivered, but payment is taken during the request. What is your suggestion?
There was a problem hiding this comment.
If the mech specifies its price and fee, why is it a mech that decides which fee goes to the marketplace?
contracts/MechMarketplace.sol
Outdated
|
|
||
| // Process payment | ||
| if (payment > 0) { | ||
| uint256 fee = IMechManager(mechManager).fee(); |
There was a problem hiding this comment.
I don't want to have multiple marketplaces. Don't like the complexity this introduces. imo it's much simpler to scope fees logic to mechs. Then marketplace interface stays the same and simply the factory supports new types of mechs. A mech specifies its fee model that gets retrieved here.
There was a problem hiding this comment.
Refactoring... but fee needs further clarification
contracts/MechMarketplace.sol
Outdated
| // Transfer payment | ||
| (bool success, ) = deliveryMech.call{value: msg.value}(""); |
There was a problem hiding this comment.
Unnecessary gas overhead no? why not make it pull rather than push. I.e. just have a mapping that's mech specific that contains their accrued fees.
contracts/MechManager.sol
Outdated
| function increaseRequestsCounts(address requester) external { | ||
| // Check for marketplace access | ||
| if (!mapMechMarketplaces[msg.sender]) { | ||
| revert UnauthorizedAccount(msg.sender); | ||
| } | ||
|
|
||
| // Record the request count | ||
| mapRequestCounts[requester]++; | ||
| // Increase the number of undelivered requests | ||
| numUndeliveredRequests++; | ||
| // Increase the total number of requests | ||
| numTotalRequests++; | ||
| } | ||
|
|
||
| function increaseDeliveryCounts(address requester, address agentMech, address mechServiceMultisig) external { | ||
| // Check for marketplace access | ||
| if (!mapMechMarketplaces[msg.sender]) { | ||
| revert UnauthorizedAccount(msg.sender); | ||
| } | ||
|
|
||
| // Decrease the number of undelivered requests | ||
| numUndeliveredRequests--; | ||
| // Increase the amount of requester delivered requests | ||
| mapDeliveryCounts[requester]++; | ||
| // Increase the amount of agent mech delivery counts | ||
| mapAgentMechDeliveryCounts[agentMech]++; | ||
| // Increase the amount of mech service multisig delivered requests | ||
| mapMechServiceDeliveryCounts[mechServiceMultisig]++; |
There was a problem hiding this comment.
better on marketplace. just confusing having this manager
contracts/MechManager.sol
Outdated
| function checkMechValidity(address agentMech) external view returns (bool status) { | ||
| // TODO: shall we also check the status of the factory? | ||
| // TODO: if yes, what if the factory was de-whitelisted? all the mechs then become invalid | ||
| status = mapAgentMechFactories[agentMech] != address(0); | ||
| } |
There was a problem hiding this comment.
We do the same as in stkaing, once created its valid. Only check at creation time.
There was a problem hiding this comment.
Then anybody is able to create a factory with its mech and then deliver the request, so at least we ned to check created mechs.
contracts/MechManager.sol
Outdated
| // Record factory that created agent mech | ||
| mapAgentMechFactories[mech] = mechFactory; |
There was a problem hiding this comment.
Not needed imo.
But where are we recording the mechs that the marketplace has registered on it?
Currently there's no on-chain mapping. That makes discovery very hard.
There was a problem hiding this comment.
Let's push each mech into the set
contracts/Karma.sol
Outdated
| @@ -37,6 +37,7 @@ contract Karma { | |||
| // Contract owner | |||
| address public owner; | |||
|
|
|||
| // TODO This must be fetched from mech manager and be removed / inaccessible from here | |||
There was a problem hiding this comment.
Irrelevant now, stays as is
contracts/AgentMech.sol
Outdated
| /// @param requestId Request id. | ||
| /// @param data Self-descriptive opaque data-blob. | ||
| /// @param mechStakingInstance Mech staking instance address (optional). | ||
| /// @param mechServiceId Mech operator service Id. | ||
| function deliverToMarketplace( | ||
| address mechMarketplace, |
There was a problem hiding this comment.
Multiple marketplaces is not helpful
