audit fixes (2) #775
audit fixes (2) #775
Conversation
|
Contract comparison - from 8ac9c19 to aba2e4e
|
dex/farm/src/base_functions.rs
Outdated
| original_owner: self.blockchain().get_sc_address(), | ||
| }; | ||
|
|
||
| let migration_farm_token_nonce = if farm_token_mapper.get_token_state().is_set() { |
There was a problem hiding this comment.
no need to create new token. Use VM endpoint GetCurrentESDTNFTNonce / implement it to Rust Framework if it does not exist.
| } | ||
| } | ||
|
|
||
| if migrated_amount > 0 { |
There was a problem hiding this comment.
why do we have duplicated code ? Can't you add this into base function ?
There was a problem hiding this comment.
The farm staking contract does not import the base_functions module as there are some variables that are Farm only specific. Maybe we could rewrite this to be more generic.
There was a problem hiding this comment.
We cannot do that due to Token Attributes. I think it's difficult to rewrite the code write now. The code changes will be huge and we would have to re-audit the whole solution.
farm-staking/farm-staking/src/lib.rs
Outdated
| original_owner: self.blockchain().get_sc_address(), | ||
| }; | ||
|
|
||
| let migration_farm_token_nonce = if farm_token_mapper.get_token_state().is_set() { |
There was a problem hiding this comment.
Same as above - use GetCurrentESDTNFTNonce. Why do you have duplicated code ? Base function should resolv this - as code is exactly the same.
There was a problem hiding this comment.
The issue for the code duplication was farm_token. It has different attributes for farm and farm-staking.
I update the code: moved this to common module and provide the token_mapper as a parameter.
| if sc.is_old_farm_position(farm_position.token_nonce) { | ||
| continue; | ||
| } | ||
|
|
||
| farm_token_mapper.require_same_token(&farm_position.token_identifier); | ||
|
|
||
| let token_attributes: FarmTokenAttributes<<Self::FarmSc as ContractBase>::Api> = | ||
| farm_token_mapper.get_token_attributes(farm_position.token_nonce); | ||
|
|
There was a problem hiding this comment.
Can we optimize here the storage access for user_farm_position? After the for, settle with increase/decrease of user_farm_position.
| if sc.is_old_farm_position(farm_position.token_nonce) { | ||
| continue; | ||
| } | ||
|
|
||
| farm_token_mapper.require_same_token(&farm_position.token_identifier); | ||
|
|
||
| let token_attributes: FarmTokenAttributes<<Self::FarmSc as ContractBase>::Api> = | ||
| farm_token_mapper.get_token_attributes(farm_position.token_nonce); | ||
|
|
There was a problem hiding this comment.
Can we extract the increase/decrease of the user_farm_position outside of the for, to improve the gas consumption?
There was a problem hiding this comment.
I'll add it in the new PR.
There was a problem hiding this comment.
Actually, it can be used only for increase position, as decrease position function needs the entire looped payment to check the attributes and decrease for the original owner. I'll see if there is any improvement if we send the original_owner as a parameter, instead of the payment.
|
|
||
| if migrated_amount > 0 { | ||
| let mut user_total_farm_position = self.get_user_total_farm_position(caller); | ||
| user_total_farm_position.total_farm_position += &migrated_amount; |
There was a problem hiding this comment.
A gas improvement here could be an update instead of a get + set. (see the decrease_old_farm_positions function below).
self.get_user_total_farm_position() also verifies if position is empty, so a new function could do that (verification + update)
There was a problem hiding this comment.
I don't know if this optimizes the gas cost as much, as we still need to check if the storage is empty before the update function. And also, this would mean another function, which would increase the contract size (even if by a small margin).
Moved try_set_farm_position_migration_nonce to common modules to remove code duplication.
No description provided.