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.

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

Exchange Deployer #62

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Exchange Deployer #62

wants to merge 14 commits into from

Conversation

dorin-iancu
Copy link
Contributor

No description provided.

Copy link

github-actions bot commented Jan 22, 2024

Contract comparison - from 6436375 to 66c1591

Path                                                                                             size                  has-allocator                     has-format
mx-exchange-tools-sc
- auto-farm/auto-farm.wasm 25888 No No
- auto-pos-creator/auto-pos-creator.wasm 16388 No No
- composable-tasks/composable-tasks.wasm 9102 No No
- energy-dao/energy-dao.wasm 34728 No No
- farm-extra-rewards-wrapper/farm-extra-rewards-wrapper.wasm 27051 No No
- locked-token-pos-creator/locked-token-pos-creator.wasm 13882 No No
- exchange-deployer/exchange-deployer.wasm N/A ➡️ 10973 ⚠️ N/A ➡️ No ⚠️ N/A ➡️ No ⚠️

@dorin-iancu dorin-iancu marked this pull request as ready for review January 22, 2024 14:24
@dorin-iancu dorin-iancu changed the base branch from main to group-arguments January 23, 2024 07:39
reward_token_id,
farming_token_id,
division_safety_constant,
pair_contract_address,
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the pair contract needed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, the farm requests it on deploy. My guess is it sends the tokens there instead of burning them if it can.

Base automatically changed from group-arguments to main January 25, 2024 07:15
fn deployed_contracts(
&self,
user_address: &ManagedAddress,
) -> UnorderedSetMapper<ManagedAddress>;
Copy link
Contributor

Choose a reason for hiding this comment

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

All the deployed contracts are stored here, but there is no info about the SC type. Should there be a info stored for each deployed address that specify the SC type?

.execute_on_dest_context();
}

#[endpoint(proxyDexSetTransferRoleWrappedLpToken)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Anyone can call these endpoints? Is this safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only the deployer of the contract can call this:
self.require_deployed_contract(&caller, &proxy_dex_address);

/// `0` - SimpleFarm - rewards are fungible tokens
/// `1` - FarmWithLockedRewards - rewards are META ESDT locked tokens
#[endpoint(simpleLockAddFarmToWhitelist)]
fn simple_lock_add_farm_to_whitelist(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same. Maybe some admin roles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.


#[multiversx_sc::module]
pub trait CommonModule {
fn get_default_code_metadata(&self) -> CodeMetadata {
Copy link
Contributor

Choose a reason for hiding this comment

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

Receive the address as a parameter and use the new get_code_metadata() framework function instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, no address as parameter, this is just the default value I use.

+ multiversx_sc_modules::pause::PauseModule
{
#[init]
fn init(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not store all these addresses unde the same storage, but using a DeployActionType enum value as the key. You will remove a lot of different storages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like the customActionFee storage.

#[view(getActionFee)]
fn get_action_fee(&self, action_type: DeployActionType) -> BigUint {
let custom_fee = self.custom_action_fee(action_type).get();
if custom_fee > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this a request? This means that you cannot have 0 fee, even if you wanted to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so.

}

#[storage_mapper("deployedContracts")]
fn deployed_contracts(
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, from what I saw from the general ProxyDeployer SC, having the information about all the deployed contracts of a specific type would be very helpful (for the microservice). Maybe add a new storage, as Costin suggested, having all the deployed contracts under a contract type key.

pub mod fee;

#[multiversx_sc::contract]
pub trait ExchangeDeployer:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have an only_owner endpoint, that allows the owner of the exchange deployer to call any only_owner endpoint of the deployed contracts. Right now you can only deploy SCs and maybe whitelist the deployer as an admin. But in case we want to remove the deployer from the admin whitelist (for being malicious let's say), you cannot do so. And this extends to all kinds of use cases, where we want to call owner endpoints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants