Skip to content
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.

Sign up for GitHub

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

test: enhancing tests #64

Merged
merged 7 commits into from
Dec 26, 2024
Merged

test: enhancing tests #64

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6e6e87c
test: enhancing tests
kupermind Dec 24, 2024
2297cb8
chore: linters
kupermind Dec 24, 2024
5c3350b
refactor: fee formula as suggested
kupermind Dec 25, 2024
0a3e4b7
fix: delivery recording bug
kupermind Dec 26, 2024
cb283c8
fix: more bugs
kupermind Dec 26, 2024
eeb2093
chore: linters
kupermind Dec 26, 2024
ba5f96e
Merge pull request #65 from valory-xyz/tests2
DavidMinarsch Dec 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .solhint.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"extends": "solhint:recommended",
"plugins": [],
"rules": {
"compiler-version": ["warn", "^0.8.25"],
"compiler-version": ["warn", "^0.8.28"],
"func-visibility": ["warn",{"ignoreConstructors":true}],
"no-empty-blocks": "off",
"not-rely-on-time": "off",
Expand Down
13 changes: 9 additions & 4 deletions contracts/BalanceTrackerFixedPriceBase.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,14 +202,19 @@ abstract contract BalanceTrackerFixedPriceBase {

// Get mech balance
uint256 balance = mapMechBalances[msg.sender];
// TODO minimal balance value to account for the round-off
if (balance == 0 || balance < 10_000) {
revert InsufficientBalance(balance, 10_000);
if (balance == 0) {
revert ZeroValue();
}

// Calculate mech payment and marketplace fee
uint256 fee = IMechMarketplace(mechMarketplace).fee();
marketplaceFee = (balance * fee) / FEE_BASE;

// If requested balance is too small, charge the minimal fee
if (balance < FEE_BASE) {
marketplaceFee = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

what if balance < marketplaceFee? need to fail..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This can't happen as balance is not zero at this point of time. But that's irrelevant already.

} else {
marketplaceFee = (balance * fee) / FEE_BASE;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@DavidMinarsch will this work as a small amounts withdraw protection?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest this instead:

marketplaceFee = (balance * fee + (FEE_BASE - 1)) / FEE_BASE;

This uses
\text{ceil}(a, b) = \frac{a + b - 1}{b}
formula

Copy link
Contributor

Choose a reason for hiding this comment

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

It will always return at least one for balance >0.

mechPayment = balance - marketplaceFee;

// Check for zero value, although this must never happen
Expand Down
58 changes: 27 additions & 31 deletions contracts/MechMarketplace.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ contract MechMarketplace is IErrorsMarketplace {
// Domain separator type hash
bytes32 public constant DOMAIN_SEPARATOR_TYPE_HASH =
keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");
// Fee base constant
uint256 public constant FEE_BASE = 10_000;
DavidMinarsch marked this conversation as resolved.
Show resolved Hide resolved

// Original domain separator value
bytes32 public immutable domainSeparator;
// Original chain Id
Expand Down Expand Up @@ -157,8 +160,8 @@ contract MechMarketplace is IErrorsMarketplace {
}

// Check for fee value
if (newFee > 10_000) {
revert Overflow(newFee, 10_000);
if (newFee > FEE_BASE) {
revert Overflow(newFee, FEE_BASE);
}

// Check for sanity values
Expand Down Expand Up @@ -314,6 +317,11 @@ contract MechMarketplace is IErrorsMarketplace {

// Traverse all the mech types and balanceTrackers
for (uint256 i = 0; i < paymentTypes.length; ++i) {
// Check for zero value
if (paymentTypes[i] == 0) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Need to check zero values here

revert ZeroValue();
}

// Check for zero address
if (balanceTrackers[i] == address(0)) {
revert ZeroAddress();
Expand Down Expand Up @@ -346,15 +354,16 @@ contract MechMarketplace is IErrorsMarketplace {
}
_locked = 2;

// responseTimeout bounds
if (responseTimeout < minResponseTimeout || responseTimeout > maxResponseTimeout) {
revert OutOfBounds(responseTimeout, minResponseTimeout, maxResponseTimeout);
}
// responseTimeout limits
// Response timeout limits
if (responseTimeout + block.timestamp > type(uint32).max) {
revert Overflow(responseTimeout + block.timestamp, type(uint32).max);
}

// Response timeout bounds
if (responseTimeout < minResponseTimeout || responseTimeout > maxResponseTimeout) {
revert OutOfBounds(responseTimeout, minResponseTimeout, maxResponseTimeout);
}

Comment on lines +362 to +366
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No change, just swapped places

// Check for non-zero data
if (data.length == 0) {
revert ZeroValue();
Expand Down Expand Up @@ -438,7 +447,7 @@ contract MechMarketplace is IErrorsMarketplace {
revert ZeroAddress();
}

// Check that the request is not already delivered
// Check if request has been delivered
if (mechDelivery.deliveryMech != address(0)) {
revert AlreadyDelivered(requestId);
}
Comment on lines +451 to 454
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently this never happens, but let's leave it here for possible mechs change.

Expand Down Expand Up @@ -517,38 +526,19 @@ contract MechMarketplace is IErrorsMarketplace {
));
}

/// @dev Checks for service validity.
/// @param serviceId Service Id.
/// @return multisig Service multisig address.
function checkServiceAndGetMultisig(
uint256 serviceId
) public view returns (address multisig) {
// Check mech service Id
IServiceRegistry.ServiceState state;
(, multisig, , , , , state) = IServiceRegistry(serviceRegistry).mapServices(serviceId);
if (state != IServiceRegistry.ServiceState.Deployed) {
revert WrongServiceState(uint256(state), serviceId);
}
}

/// @dev Checks for mech validity.
/// @param mech Mech contract address.
/// @return multisig Mech service multisig address.
function checkMech(address mech) public view returns (address multisig){
function checkMech(address mech) public view returns (address multisig) {
uint256 mechServiceId = IMech(mech).tokenId();

// Check mech validity as it must be created and recorded via this marketplace
if (mapServiceIdMech[mechServiceId] != mech) {
revert UnauthorizedAccount(mech);
}

// Check mech service Id
multisig = checkServiceAndGetMultisig(mechServiceId);

// Check that service multisig is the priority mech service multisig
if (!IMech(mech).isOperator(multisig)) {
revert UnauthorizedAccount(mech);
}
// Check mech service Id and get its multisig
multisig = IMech(mech).getOperator();
Comment on lines +533 to +542
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is now more than enough, the rest was code duplication.

}

/// @dev Checks for requester validity.
Expand All @@ -561,7 +551,13 @@ contract MechMarketplace is IErrorsMarketplace {
) public view {
// Check for requester service
if (requesterServiceId > 0) {
address multisig = checkServiceAndGetMultisig(requesterServiceId);
(, address multisig, , , , , IServiceRegistry.ServiceState state) =
IServiceRegistry(serviceRegistry).mapServices(requesterServiceId);

// Check for correct service state
if (state != IServiceRegistry.ServiceState.Deployed) {
revert WrongServiceState(uint256(state), requesterServiceId);
}

// Check staked service multisig
if (multisig != requester) {
Expand Down
Loading
Loading