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!: add methods() to contract abigen #592

Merged
merged 4 commits into from
Sep 28, 2022

Conversation

hal3e
Copy link
Contributor

@hal3e hal3e commented Sep 26, 2022

closes: #515

  • removed the Builder pattern from abigen
  • added a new struct that holds the contract methods which is built by the contract instance

Now the workflow looks like this:

    abigen!(
        SimpleContract,
        "packages/fuels/tests/takes_ints_returns_bool-abi.json",
    );

    let wallet = launch_provider_and_get_wallet().await;

    // `SimpleContract` is the name of the contract
    let contract_instance = SimpleContract::new(null_contract_id(), wallet);

    let call_handler = contract_instance.methods().takes_ints_returns_bool(42);

In the tests where we have multiple contract calls, I have saved the methods in a new variable and called the methods directly. For example:

    let contract_methods = SimpleContract::new(null_contract_id(), wallet).methods();

    let call_handler = contract_methods.takes_nested_struct(nested_struct_from_tokens);

@hal3e hal3e requested a review from a team September 26, 2022 14:59
@hal3e hal3e self-assigned this Sep 26, 2022
@hal3e hal3e requested a review from iqdecay as a code owner September 26, 2022 14:59
@hal3e
Copy link
Contributor Author

hal3e commented Sep 26, 2022

While I was updating the harness tests I figured that I could also update some test that I did not update when I made the setup_test_contract! macro. At that time, I did not update them because they used some custom wallets and/or providers. But then I remembered that if I named the custom wallet shared_wallet and declare it before the macro, I can still use the macro with the None value. But now I do not like the name shared_wallet anymore 😄 (Initially, I put shared_wallet as it would be shared between macros but that is not the only use case). So I decided to rename shared_wallet to just wallet and now we can have custom wallets or providers and still use the macro. Here is an example:

    let config = WalletsConfig::new(Some(2), Some(1), Some(DEFAULT_COIN_AMOUNT));

    let mut wallets = launch_custom_provider_and_get_wallets(config, None).await;
    let wallet = wallets.pop().unwrap();
    let wallet_2 = wallets.pop().unwrap();

    setup_contract_test!(
        contract_instance,
        None,
        "packages/fuels/tests/test_projects/contract_test"
    );

Let me know what you think about this 😄

Copy link
Member

@Salka1988 Salka1988 left a comment

Choose a reason for hiding this comment

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

Nice! 👍

docs/src/contracts/the-setup-contract-test-macro.md Outdated Show resolved Hide resolved
Copy link
Contributor

@segfault-magnet segfault-magnet left a comment

Choose a reason for hiding this comment

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

Love it.

docs/src/contracts/the-setup-contract-test-macro.md Outdated Show resolved Hide resolved
docs/src/contracts/the-setup-contract-test-macro.md Outdated Show resolved Hide resolved
self
}

pub fn wallet(&mut self, wallet: WalletUnlocked) -> &mut Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this and with_wallet? I understand that with_wallet returns a new instance, but having them both is confusing. We should at least rename them, although I am more for deleting one unless there is an explicit need for it.

let provider = self.wallet.get_provider()?;
wallet.set_provider(provider.clone());

Ok(Self { contract_id: self.contract_id.clone(), wallet: wallet })
}

pub fn methods(&self) -> #methods_name {
#methods_name { contract_id: self.contract_id.clone(), wallet: self.wallet.clone() }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have a reference to the instance instead? What should the user expect to happen if they change the wallet of the instance with the above wallet() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They must call methods() again. AFAIK rust will not allow to have a reference to a dropped wallet. As the new one would replace it.

Copy link
Contributor

Choose a reason for hiding this comment

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

This creates a pitfall. It's not evident that if you change the wallet, the methods are affected too.
Can we use a reference to the actual instance or add a warning to the docs?

Copy link
Contributor Author

@hal3e hal3e Sep 27, 2022

Choose a reason for hiding this comment

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

(I am writing this without trying it out). wallet() and with_wallet both create a new instance so I think the reference is again a no go. IMO documentation should be enough.
wallet() does not create a new instance. We have two options:

  • Try to have a reference inside methods to the instance. (I do not know if this will make problems when wallet moves)
  • Remove wallet() and contract_id() (you can always make a new instance)

I am leaning toward the second option. If you use with_wallet you create a new instance and have to call methods again..

For context, I copied the wallet() and contract_id() methods from the old builder.

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 implemented the second option.

@digorithm digorithm added the breaking Introduces or requires breaking changes label Sep 27, 2022
Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

Super clean implementation; goodbye name conflicts! 🎉

@hal3e hal3e changed the title refactor: add methods() to contract abigen refactor!: add methods() to contract abigen Sep 28, 2022
@hal3e hal3e merged commit b5c74eb into master Sep 28, 2022
@hal3e hal3e deleted the hal3e/abigen-contract-methods branch September 28, 2022 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Introduces or requires breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collision between contract instance- and binding methods
5 participants