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

[test] Enable storage limit for emulator backend #271

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

Conversation

m-Peter
Copy link
Contributor

@m-Peter m-Peter commented Jan 8, 2024

Description

This will make the testing environment behave more similar to a development/testnet/mainnet environment.


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

This will make the testing environment behave more similar to
a development/testnet/mainnet environment.
@m-Peter m-Peter marked this pull request as draft January 8, 2024 17:54
@m-Peter
Copy link
Contributor Author

m-Peter commented Jan 8, 2024

Note: I will rework this to be enabled with a pragma, instead of enabling it by default.
That is actually requires quite some changes, and I do not see any benefit. I think it is better to have the storage limit always enabled, as users can easily mint/burn flows, to test out scenarios with storage.

@m-Peter m-Peter marked this pull request as ready for review January 11, 2024 10:23
Copy link
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

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

Is there a way to set the storage limit for tests? Or does it use a default value always?

@m-Peter
Copy link
Contributor Author

m-Peter commented Jan 15, 2024

The default value is taken from here: https://github.com/onflow/flow-go/blob/47e239c3b505d75b41c3cb92666eba56d541c8a0/fvm/bootstrap.go#L26-L28, when using emulator.WithStorageLimitEnabled(true).
We can specify a different value with emulator.WithMinimumStorageReservation(), but this cannot be changed once the emulator is up and running. That's why I went with the default value, to keep parity with the rest of the environments.

@@ -735,7 +735,7 @@ func TestImportContract(t *testing.T) {
Test.assert(blockHeight > 1)

mintFlow(to: admin, amount: 500.0)
Test.assertEqual(500.0, getFlowBalance(account: admin))
Test.assertEqual(500.001, getFlowBalance(account: admin))
Copy link
Member

Choose a reason for hiding this comment

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

I feel these small value differences in account balances could be annoying for users when writing tests.
(Also not sure why it changes the account balances when the storage limit is enabled).

So maybe we should have this disabled by default (to make the simple use-cases simple), and allow developers to enable it only if they need it.

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 feel these small value differences in account balances could be annoying for users when writing tests.

That' a good point 👍

(Also not sure why it changes the account balances when the storage limit is enabled).

The emulator.WithStorageLimitEnabled(true) means that every account has to be created with the minimum storage reservation (0.001 FLOW), otherwise the account would break the storage limit constraint.

So maybe we should have this disabled by default (to make the simple use-cases simple), and allow developers to enable it only if they need it.

Sounds good, I will try the approach where this is enabled per test file, with a pragma directive (#storageLimitEnabled).

@m-Peter m-Peter requested a review from SupunS January 22, 2024 10:13
@m-Peter
Copy link
Contributor Author

m-Peter commented Jan 22, 2024

@SupunS I have reworked this to use a Cadence pragma directive, instead of enabling it by default. Take a look when you can, and let me know what you think 🙏

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