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

refactor child contracts #4

Merged
merged 2 commits into from
Oct 12, 2023
Merged

refactor child contracts #4

merged 2 commits into from
Oct 12, 2023

Conversation

dorin-iancu
Copy link
Contributor

  • remove EthAddress type
  • remove fees logic
  • remove EthTransaction -> use Transaction everywhere. Also add additional fields to allow cross-chain SC calls.
  • refactor to allow multi-token transfers (max 10 per call atm)
  • refactor to allow SFT/NFT transfers

Multi-sig SC will be fixed in a future PR.

ticker: ManagedBuffer,
opt_default_price_per_gas_unit: OptionalValue<BigUint>,
) {
fn add_token_to_whitelist(&self, token_id: TokenIdentifier, ticker: ManagedBuffer) {

Choose a reason for hiding this comment

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

Can you extract ticker from token_id so token_ticker storage would not be necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, we likely don't need ticker at all anymore. Future PR.

#[derive(
TopDecode, TopEncode, NestedDecode, NestedEncode, TypeAbi, Debug, ManagedVecItem, Clone,
)]
pub struct StolenFromFrameworkEsdtTokenData<M: ManagedTypeApi> {

Choose a reason for hiding this comment

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

Can you rename StolenFromFrameworkEsdtTokenData?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be introduced in framework in the next version. Will be deleted then :)

esdt-safe/src/lib.rs Outdated Show resolved Hide resolved
self.burn_esdt_token(&tx.token_identifier, &tx.amount);
for token in &tx.tokens {
if self.is_local_role_set(&token.token_identifier, &EsdtLocalRole::Burn) {
self.send().esdt_local_burn(

Choose a reason for hiding this comment

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

Does the esdt-safe SC have the roles to burn any deposited token?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why it's checked first :)

@@ -41,15 +42,10 @@ pub trait EsdtSafe:
self.first_batch_id().set_if_empty(1);

Choose a reason for hiding this comment

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

I think first_batch_id should be renamed with current_batch_id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh, maybe, but not for now. Let's leave it as it is.

@@ -103,50 +106,30 @@ pub trait EsdtSafe:
self.clear_first_batch(&mut tx_batch);
}

/// Converts failed Ethereum -> Elrond transactions to Elrond -> Ethereum transaction.

Choose a reason for hiding this comment

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

Elrond -> MultiversX

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rebranding can be done by someone else :)


let own_sc_address = self.blockchain().get_sc_address();
let mut all_token_data = ManagedVec::new();
for payment in &payments {

Choose a reason for hiding this comment

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

Verify gas consumption in case of too many payments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a max transfer limit and a max gas limit too in a future PR.

@@ -229,12 +226,19 @@ pub trait EsdtSafe:
fn get_refund_amounts(
&self,
address: ManagedAddress,
) -> MultiValueEncoded<MultiValue2<TokenIdentifier, BigUint>> {
) -> MultiValueEncoded<MultiValue3<TokenIdentifier, u64, BigUint>> {
let mut refund_amounts = MultiValueEncoded::new();
for token_id in self.token_whitelist().iter() {

Choose a reason for hiding this comment

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

Save self.token_whitelist() in a variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need, the call is done only once.

let mut tokens_to_send = ManagedVec::new();
let mut sent_token_data = ManagedVec::new();

for (token, opt_token_data) in sov_tx.tokens.iter().zip(sov_tx.token_data.iter()) {

Choose a reason for hiding this comment

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

create a variable for sov_tx.tokens.iter().zip(sov_tx.token_data.iter())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need again, it's only called once.

@dorin-iancu dorin-iancu merged commit 27505dd into main Oct 12, 2023
1 of 4 checks passed
@dorin-iancu dorin-iancu deleted the remove-eth-addr branch October 12, 2023 06:23
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.

2 participants