[MEX-580] Add on behalf features for farm SC #967
Merged
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 e934c85
|
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.
dex/farm/src/external_interaction.rs
Outdated
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
There was a problem hiding this comment.
Updated for farms and farm staking. Farm staking proxy is too custom to be handled the same way.
There was a problem hiding this comment.
Also, added a new PermissionsHubModule, to remove duplicated code.
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?
There was a problem hiding this comment.
Updated for farms and farm staking. Farm staking proxy is too custom to be handled the same way.
There was a problem hiding this comment.
Also, added a new PermissionsHubModule, to remove duplicated code.
dorin-iancu
reviewed
Nov 20, 2024
CostinCarabas
previously approved these changes
Nov 21, 2024
dorin-iancu
previously approved these changes
Nov 21, 2024
psorinionut
changed the title
[MEX-580] add readme for permissions hub SC + onBehalf features
ovidiuolteanu
dismissed stale reviews from dorin-iancu and CostinCarabas
via
a2445d3
bump mx-sc-actions version
ovidiuolteanu
approved these changes
Jan 10, 2025
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: