-
Notifications
You must be signed in to change notification settings - Fork 45
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
Affiliates v2 #233
Affiliates v2 #233
Conversation
…est for this case
The coveralls shows failure due to decreased coverage, saw these two files mainly contributing to the cause. These are the files where the coverage is decreased (you can use the key Coveralls failure is more towards quality than test failure, if you could cover those edge cases as well, that would be nice. |
Another merge with |
https://github.com/DistributedCollective/Sovryn-smart-contracts/tree/development/contracts/locked This is in |
It's used to simulate the failed process of depositSOV in lockedSOV contract. In case it's failed, the affiliates contract will log it. |
Remove flashloan whitelisting code
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.
only minor issues
address affiliateReferrer, // the user was brought by the affiliate (referrer) | ||
bytes memory loanDataBytes // arbitrary order data | ||
) | ||
public |
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.
external
|
||
event SetProtocolAddress(address indexed sender, address indexed oldProtocol, address indexed newProtocol); | ||
|
||
event SetMinReferralsToPayoutAffiliates(address indexed sender, uint256 indexed oldMinReferrals, uint256 indexed newMinReferrals); |
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.
should oldMinReferrals
/newMinReferrals
be indexed
?
*/ | ||
function enumerate(AddressSet storage set) internal view returns (address[] memory) { | ||
address[] memory output = new address[](set.values.length); | ||
for (uint256 i; i < set.values.length; i++) { |
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.
set.values.length
reads storage in a loop
contracts/mixins/FeesHelper.sol
Outdated
address feeToken, | ||
uint256 tradingFee | ||
) internal returns (uint256 affiliatesBonusSOVAmount, uint256 affiliatesBonusTokenAmount) { | ||
(affiliatesBonusSOVAmount, affiliatesBonusTokenAmount) = ProtocolAffiliatesInterface(protocolAddress).payTradingFeeToAffiliatesReferrer( |
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.
you can get away with unnamed return parameters and just return ProtocolAffiliatesInterface(...).payTradingFeeToAffiliatesReferrer(...)
* @dev settles the trading fee and pays the token reward to the user. | ||
* @param referrer the affiliate referrer address to send the reward to | ||
* @param feeToken the address of the token in which the trading fee is paid | ||
* */ |
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.
natspec is missing return values
constructor() public {} | ||
|
||
function() external { | ||
revert("Affiliates - fallback not allowed"); |
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.
fallback with revert is unnecessary, unless you want custom revert message
contracts/modules/Affiliates.sol
Outdated
} | ||
} | ||
|
||
function getReferralsList(address referrer) public view returns (address[] memory refList) { |
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.
external
contracts/modules/Affiliates.sol
Outdated
IPriceFeeds(_priceFeeds).queryReturn.selector, | ||
feeToken, | ||
sovTokenAddress, // dest token = SOV | ||
feeAmount.mul(_getAffiliatesTradingFeePercentForSOV()).div(10**20) |
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.
10**20 -> 1e20
feeAmount.mul(_getAffiliatesTradingFeePercentForSOV()).div(10**20) | ||
) | ||
); | ||
assembly { |
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.
is abi.decode
available? it's a cleaner way to parse bytes
contracts/modules/Affiliates.sol
Outdated
address token, | ||
uint256 tradingFeeTokenBaseAmount | ||
) external onlyCallableInternal returns (uint256 referrerBonusSovAmount, uint256 referrerBonusTokenAmount) { | ||
bool isHeld = referralsList[referrer].length() >= getMinReferralsToPayout() ? false : true; |
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.
bool isHeld = referralsList[referrer].length() < getMinReferralsToPayout();
in general, true
and false
are only required in single-word statements like return true;
, while(true)
or someFlag = false;
Please test this on testnet first before merging with Please review the testing on testnet @tjcloa |
Need to create new PR for this feature due to lack of testing on testnet. |
Affiliates V2 done with LockedSOV implementation.
So the rewards for referrer will be stored in LockedSOV contract.
For the deployment, can use the deployAffiliate() in contract_interaction.py