-
Notifications
You must be signed in to change notification settings - Fork 1
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
refactor and fix: addressing recent changes #62
Conversation
kupermind
commented
Dec 23, 2024
- Addressing recent changes.
@@ -35,7 +35,14 @@ contract BalanceTrackerFixedPriceNative is BalanceTrackerFixedPriceBase { | |||
wrappedNativeToken = _wrappedNativeToken; | |||
} | |||
|
|||
function _checkNativeValue() internal virtual override {} | |||
function _getOrRestrictNativeValue() internal virtual override returns (uint256) { |
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.
This is the current refactor of incoming payment.
} | ||
|
||
function _getRequiredFunds(address requester, uint256 balanceDiff) internal virtual override returns (uint256) { | ||
// TODO check balances before and after | ||
uint256 balanceBefore = IToken(olas).balanceOf(address(this)); |
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.
Additional check, although with OLAS it's pretty much safe by default.
// Get salt | ||
bytes32 salt = keccak256(abi.encode(block.timestamp, msg.sender, serviceId)); | ||
bytes32 salt = keccak256(abi.encode(block.timestamp, msg.sender, serviceId, localNonce)); |
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.
Adding nonce as per audit suggestion
@@ -5,13 +5,16 @@ import {OlasMech} from "./OlasMech.sol"; | |||
|
|||
/// @title MechFixedPriceToken - Smart contract for OlasMech that accepts a fixed price payment for services in native token. | |||
contract MechFixedPriceToken is OlasMech { | |||
// keccak256(FixedPriceToken) = 3679d66ef546e66ce9057c4a052f317b135bc8e8c509638f7966edfd4fcf45e9 | |||
bytes32 public constant PAYMENT_TYPE = 0x3679d66ef546e66ce9057c4a052f317b135bc8e8c509638f7966edfd4fcf45e9; |
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.
Hashing approach always wins :). Although in this case it feels like we maybe need to call it FixedPriceOlas? Either way is fine.
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.
hm I think the token can be different kinds for the same factory contract (different factory instance) no?
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.
Makes sense, maybe encode it on a level of factory? There we can combine FixedPriceToken
+ token address or anything else.
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.
Hm, I'll think about it.
// Increase the amount of mech delivery counts | ||
mapAgentMechDeliveryCounts[msg.sender]++; | ||
mapMechDeliveryCounts[msg.sender]++; | ||
// Increase the amount of mech service multisig delivered requests | ||
mapMechServiceDeliveryCounts[mechServiceMultisig]++; |
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.
We still need both - one is for mech statistics, another is for multisig activity tracker.
/// @dev Gets the requests count for a specific account. | ||
/// @param account Account address. | ||
/// @return Requests count. | ||
function getRequestsCount(address account) external view returns (uint256) { |
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.
Default ABI is enough