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.

Sign up for GitHub

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

Jump to bottom

Balance integration fix #1982

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

Balance integration fix #1982

wants to merge 3 commits into from

Conversation

faculerena
Copy link
Contributor

@faculerena faculerena commented Nov 9, 2023 •
edited
Loading

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

ascjones
ascjones reviewed Nov 13, 2023
View reviewed changes
crates/env/src/engine/off_chain/test_api.rs Outdated
@@ -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?

@XianPaz XianPaz mentioned this pull request Nov 13, 2023
Open
@faculerena
Changed the default account balance to the testnet genesis default
8ec3b37 
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 •
edited
Loading

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
Reviewers

@cmichi cmichi Awaiting requested review from cmichi cmichi is a code owner

@SkymanOne SkymanOne Awaiting requested review from SkymanOne

@ascjones ascjones Awaiting requested review from ascjones ascjones is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@faculerena @cmichi @ascjones