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

Balance integration fix #1982

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

faculerena
Copy link
Contributor

@faculerena faculerena commented Nov 9, 2023

Summary

  • [N] y/n | Does it introduce breaking changes?
  • [N] y/n | Is it dependant on the specific version of cargo-contract or pallet-contracts?

Updated the default balance in integration tests.

Description

We updated the default balance in integration tests to be the same as in e2e-tests. This is not definitive, as we want opinions if this should be like this.

Checklist before requesting a review

  • My code follows the style guidelines of this project
  • I have added an entry to CHANGELOG.md
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

@@ -306,7 +306,8 @@ where
instance.engine.set_callee(encoded_alice.clone());

// set up the funds for the default accounts
let substantial = 1_000_000;
// the 1_000_000_000 is the same value as in the e2e tests
let substantial = 1_000_000_000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the original 1_000_000 value was somewhat arbitrary anyway, so makes sense to change it to be the same as the substrate-contracts-node used for testsin.

Is it only the Alice account that is endowed with this amount? I see bob and charlie are still set to 1_000 below - does that match up with the endowed amounts for the e2e tests too? Seems kind of low.

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 true, i will check the behaviour and make it equal for all name accounts (at least). The accounts created with ::From([1;32]) should have a balance of 0 or the existential minimum?

This is the same value as substrate-contracts-node/node/src/chain_spec.rs `test_genesis` function
@faculerena
Copy link
Contributor Author

faculerena commented Nov 15, 2023

I changed the value to be the same as the testnet genesis. This is the same value that this test gives in an e2e test.

  let alice_balance = client
      .balance(ink_e2e::account_id(ink_e2e::AccountKeyring::Alice))
      .await
      .expect("Failed to get account balance");

In this case, alice_balance is (1 << 60), and for each account in the keyring this is true too, except for one and two, that is 0.

@cmichi
Copy link
Collaborator

cmichi commented Nov 13, 2024

@faculerena Could you merge master into your PR? There are some conflicts that have to be resolved.

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.

3 participants