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

feat(gtest): Improve value and ED managment #4111

Merged
merged 35 commits into from
Aug 19, 2024

Conversation

ByteNacked
Copy link
Member

@ByteNacked ByteNacked commented Aug 4, 2024

Release notes:

  • Improved value and ED management in gtest. Programs now spend gas during execution and charge ED upon creation.
  • Refactored gtest, adding separate Accounts and Actors stores.
  • Removed the Program::mint method from gtest; users should now use System::transaction to top up a program's balance.

@reviewer-or-team

@ByteNacked ByteNacked self-assigned this Aug 4, 2024
@ByteNacked ByteNacked added A0-pleasereview PR is ready to be reviewed by the team and removed A0-pleasereview PR is ready to be reviewed by the team labels Aug 4, 2024
@ByteNacked ByteNacked marked this pull request as draft August 4, 2024 23:34
gtest/src/manager.rs Outdated Show resolved Hide resolved
@ByteNacked ByteNacked force-pushed the rmasl-gtest-improve-value-managment branch from 8958553 to c5df457 Compare August 10, 2024 19:54
@ByteNacked ByteNacked force-pushed the rmasl-gtest-improve-value-managment branch from c5df457 to 6d8115d Compare August 12, 2024 01:23
@ByteNacked ByteNacked changed the base branch from master to st-block-exec-gtest August 12, 2024 09:54
@ByteNacked
Copy link
Member Author

ByteNacked commented Aug 12, 2024

TODO:

  • Fix rest tests
  • Add additional checks in balance module, improve panic logs

gtest/src/lib.rs Outdated Show resolved Hide resolved
gtest/src/log.rs Outdated Show resolved Hide resolved
gtest/src/program.rs Outdated Show resolved Hide resolved
gtest/src/program.rs Outdated Show resolved Hide resolved
gtest/src/manager.rs Outdated Show resolved Hide resolved
gtest/src/balance.rs Outdated Show resolved Hide resolved
gtest/src/manager.rs Outdated Show resolved Hide resolved
gtest/src/balance.rs Outdated Show resolved Hide resolved
gtest/src/balance.rs Outdated Show resolved Hide resolved
gtest/src/manager.rs Outdated Show resolved Hide resolved
@ByteNacked
Copy link
Member Author

Fixed all tests

@ByteNacked ByteNacked added the A0-pleasereview PR is ready to be reviewed by the team label Aug 13, 2024
@ByteNacked ByteNacked changed the title feat(gtest): Improve value managment feat(gtest): Improve value and ED managment Aug 13, 2024
Base automatically changed from st-block-exec-gtest to master August 14, 2024 08:46
Copy link
Member

@techraed techraed left a comment

Choose a reason for hiding this comment

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

You need to rebase to master

Copy link
Member

@techraed techraed left a comment

Choose a reason for hiding this comment

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

Excellent work ⭐
Love it how gtest looks now.

There are some tiny correction has to be done. Also very important:

  1. actors and accounts storages must be cleaned when System is dropped (see drop impl for the System)
  2. I suggest removing Program::mint_to. Actually System::transfer is the most right way to mint balance to the program. Also I'd remove ED check from the ExtManager::mint_to, because 1) the Account::increase already does the op the proper way (if account exists then allows minting value lt ED), 2) the method would be used only for users, not programs (see 1).
  3. In accordance to 2 need to add check that id is the user in System::mint_to.

gtest/src/lib.rs Outdated Show resolved Hide resolved
gtest/src/lib.rs Outdated Show resolved Hide resolved
gtest/src/lib.rs Outdated Show resolved Hide resolved
gtest/src/lib.rs Outdated Show resolved Hide resolved
gtest/src/lib.rs Outdated Show resolved Hide resolved
gtest/src/accounts.rs Show resolved Hide resolved
gtest/src/accounts.rs Outdated Show resolved Hide resolved
gtest/src/bank.rs Outdated Show resolved Hide resolved
gtest/src/manager.rs Outdated Show resolved Hide resolved
gtest/src/manager.rs Outdated Show resolved Hide resolved
Copy link
Member

@techraed techraed left a comment

Choose a reason for hiding this comment

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

🔥

gtest/src/accounts.rs Outdated Show resolved Hide resolved
@techraed techraed added A2-mergeoncegreen PR is ready to merge after CI passes B1-releasenotes The feature deserves to be added to the Release Notes and removed A0-pleasereview PR is ready to be reviewed by the team labels Aug 19, 2024
@ByteNacked ByteNacked merged commit e6c6b25 into master Aug 19, 2024
10 checks passed
@ByteNacked ByteNacked deleted the rmasl-gtest-improve-value-managment branch August 19, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A2-mergeoncegreen PR is ready to merge after CI passes B1-releasenotes The feature deserves to be added to the Release Notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants