Add on behalf features for farm SC #967
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Coverage SummaryTotals
FilesExpand
|
|
Contract comparison - from 79c6016 to 17b9601
|
dorin-iancu
reviewed
Nov 4, 2024
Comment on lines
+54
to
+56
| #[view(getBlacklistedAddresses)] | ||
| #[storage_mapper("blacklistedAddresses")] | ||
| fn blacklisted_addresses(&self) -> UnorderedSetMapper<ManagedAddress>; |
There was a problem hiding this comment.
Why would you ever need a blacklist if all actions require user to be whitelisted?
There was a problem hiding this comment.
I've never even seen this blacklist used anywhere in the code, so I'd say just remove it.
There was a problem hiding this comment.
The blacklist is used in is_whitelisted view. The blacklist here is design to override the user's whitelist. For example, if the users whitelist an address and that address becomes malicious in any way, we as owners would have a way to block that address generally without clearing it out of everyone's whitelists.
| #[upgrade] | ||
| fn upgrade(&self) {} | ||
|
|
||
| #[endpoint(whitelist)] |
There was a problem hiding this comment.
Suggested change
| #[endpoint(whitelist)] | |
| #[endpoint] |
No need to specify the name if it's the same as the function.
Comment on lines
+15
to
+26
| fn whitelist(&self, address_to_whitelist: ManagedAddress) { | ||
| let caller = self.blockchain().get_caller(); | ||
| self.user_whitelisted_addresses(&caller) | ||
| .insert(address_to_whitelist); | ||
| } | ||
|
|
||
| #[endpoint(removeWhitelist)] | ||
| fn remove_whitelist(&self, address_to_remove: ManagedAddress) { | ||
| let caller = self.blockchain().get_caller(); | ||
| self.user_whitelisted_addresses(&caller) | ||
| .swap_remove(&address_to_remove); | ||
| } |
There was a problem hiding this comment.
I suggest taking MultiValueEncoded<ManagedAddress> as argument and add/remove.
There was a problem hiding this comment.
i.e.
let mapper = user_whitelisted_addresses(&caller);
for addr in addresses {
let _ = mapper.insert(&addr)
}
Similar for remove.
Comment on lines
+17
to
+18
| self.user_whitelisted_addresses(&caller) | ||
| .insert(address_to_whitelist); |
There was a problem hiding this comment.
Suggested change
| self.user_whitelisted_addresses(&caller) | |
| .insert(address_to_whitelist); | |
| let _ = self.user_whitelisted_addresses(&caller) | |
| .insert(address_to_whitelist); |
Comment on lines
+24
to
+25
| self.user_whitelisted_addresses(&caller) | ||
| .swap_remove(&address_to_remove); |
There was a problem hiding this comment.
Suggested change
| self.user_whitelisted_addresses(&caller) | |
| .swap_remove(&address_to_remove); | |
| let removed = self.user_whitelisted_addresses(&caller) | |
| .swap_remove(&address_to_remove); | |
| require!(removed, "address not in whitelist"); |
Just for a bit of extra safety.
Comment on lines
+77
to
+150
| fn check_and_return_original_owner(&self) -> ManagedAddress { | ||
| let payments = self.call_value().all_esdt_transfers().clone_value(); | ||
| let farm_token_mapper = self.farm_token(); | ||
| let mut original_owner = ManagedAddress::zero(); | ||
| for payment in payments.into_iter() { | ||
| let attributes: FarmTokenAttributes<Self::Api> = | ||
| farm_token_mapper.get_token_attributes(payment.token_nonce); | ||
|
|
||
| if original_owner.is_zero() { | ||
| original_owner = attributes.original_owner; | ||
| } else { | ||
| require!( | ||
| original_owner == attributes.original_owner, | ||
| "All position must have the same original owner" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| require!( | ||
| !original_owner.is_zero(), | ||
| "Original owner could not be identified" | ||
| ); | ||
|
|
||
| original_owner | ||
| } | ||
|
|
||
| fn check_additional_payments_original_owner(&self, user: &ManagedAddress) { | ||
| let payments = self.call_value().all_esdt_transfers().clone_value(); | ||
| if payments.len() == 1 { | ||
| return; | ||
| } | ||
|
|
||
| let farm_token_mapper = self.farm_token(); | ||
| let farm_token_id = farm_token_mapper.get_token_id(); | ||
| for payment in payments.into_iter() { | ||
| if payment.token_identifier != farm_token_id { | ||
| continue; | ||
| } | ||
|
|
||
| let attributes: FarmTokenAttributes<Self::Api> = | ||
| farm_token_mapper.get_token_attributes(payment.token_nonce); | ||
|
|
||
| require!( | ||
| user == &attributes.original_owner, | ||
| "Provided address is not the same as the original owner" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| fn require_user_whitelisted(&self, user: &ManagedAddress, authorized_address: &ManagedAddress) { | ||
| let permissions_hub_address = self.permissions_hub_address().get(); | ||
| let is_whitelisted: bool = self | ||
| .permissions_hub_proxy(permissions_hub_address) | ||
| .is_whitelisted(user, authorized_address) | ||
| .execute_on_dest_context(); | ||
|
|
||
| require!(is_whitelisted, "Caller is not whitelisted by the user"); | ||
| } | ||
|
|
||
| #[only_owner] | ||
| #[endpoint(setPermissionsHubAddress)] | ||
| fn set_permissions_hub_address(&self, address: ManagedAddress) { | ||
| self.permissions_hub_address().set(&address); | ||
| } | ||
|
|
||
| #[proxy] | ||
| fn permissions_hub_proxy( | ||
| &self, | ||
| sc_address: ManagedAddress, | ||
| ) -> permissions_hub::Proxy<Self::Api>; | ||
|
|
||
| #[storage_mapper("permissionsHubAddress")] | ||
| fn permissions_hub_address(&self) -> SingleValueMapper<ManagedAddress>; | ||
| } |
There was a problem hiding this comment.
I really don't like the fact that this code is duplicated in all the contracts. You can refactor it and put it into the common/modules/farm folder.
For check_and_return_original_owner, take the mapper as argument, and take the attributes type as a trait parameter, i.e.
fn check_and_return_original_owner<T: FarmToken>(&self, mapper: &NonFungibleTokenMapper) -> ManagedAddress {
//
}
Add a method for fn get_original_owner(&self) -> ManagedAddress in FarmToken trait, since you need that field.
Also, for consistency with the other methods, have the payments as arguments instead.
Similar changes for check_additional_payments_original_owner
There was a problem hiding this comment.
I've done what you need here, so you can copy that part: https://github.com/multiversx/mx-exchange-sc/pull/968/files
Comment on lines
+40
to
+75
| #[payable("*")] | ||
| #[endpoint(enterFarmOnBehalf)] | ||
| fn enter_farm_on_behalf(&self, user: ManagedAddress) -> EnterFarmResultType<Self::Api> { | ||
| let caller = self.blockchain().get_caller(); | ||
| self.require_user_whitelisted(&user, &caller); | ||
|
|
||
| self.check_additional_payments_original_owner(&user); | ||
|
|
||
| let boosted_rewards = self.claim_only_boosted_payment(&user); | ||
|
|
||
| let boosted_rewards_payment = | ||
| EsdtTokenPayment::new(self.reward_token_id().get(), 0, boosted_rewards); | ||
|
|
||
| let new_farm_token = self.enter_farm::<Wrapper<Self>>(user.clone()); | ||
| self.send_payment_non_zero(&caller, &new_farm_token); | ||
| self.send_payment_non_zero(&user, &boosted_rewards_payment); | ||
|
|
||
| self.update_energy_and_progress(&user); | ||
|
|
||
| (new_farm_token, boosted_rewards_payment).into() | ||
| } | ||
|
|
||
| #[payable("*")] | ||
| #[endpoint(claimRewardsOnBehalf)] | ||
| fn claim_rewards_on_behalf(&self) -> ClaimRewardsResultType<Self::Api> { | ||
| let user = self.check_and_return_original_owner(); | ||
| let caller = self.blockchain().get_caller(); | ||
| self.require_user_whitelisted(&user, &caller); | ||
|
|
||
| let claim_rewards_result = self.claim_rewards::<Wrapper<Self>>(user.clone()); | ||
|
|
||
| self.send_payment_non_zero(&caller, &claim_rewards_result.new_farm_token); | ||
| self.send_payment_non_zero(&user, &claim_rewards_result.rewards); | ||
|
|
||
| claim_rewards_result.into() | ||
| } |
There was a problem hiding this comment.
These functions can remain here, as they seem to do a lot of different logic.
Comment on lines
+86
to
+94
| let claim_result = ClaimDualYieldResult { | ||
| lp_farm_rewards, | ||
| staking_farm_rewards, | ||
| new_dual_yield_tokens, | ||
| }; | ||
|
|
||
| dual_yield_token_mapper.nft_burn(payment.token_nonce, &payment.amount); | ||
|
|
||
| claim_result |
There was a problem hiding this comment.
Suggested change
| let claim_result = ClaimDualYieldResult { | |
| lp_farm_rewards, | |
| staking_farm_rewards, | |
| new_dual_yield_tokens, | |
| }; | |
| dual_yield_token_mapper.nft_burn(payment.token_nonce, &payment.amount); | |
| claim_result | |
| dual_yield_token_mapper.nft_burn(payment.token_nonce, &payment.amount); | |
| ClaimDualYieldResult { | |
| lp_farm_rewards, | |
| staking_farm_rewards, | |
| new_dual_yield_tokens, | |
| } |
CostinCarabas
requested changes
Nov 13, 2024
| (virtual_farm_token.payment, claim_result.rewards).into() | ||
| } | ||
|
|
||
| fn check_and_return_original_owner(&self, payment: &EsdtTokenPayment) -> ManagedAddress { |
There was a problem hiding this comment.
Duplicated code with farm, farm-staking-proxy, etc.. Can you refactor to avoid code duplication?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Features: