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

near-sdk-sim 3.2.0: Expose cur block and fix storage costs #390

Merged
merged 9 commits into from
May 3, 2021

Conversation

evgenykuzyakov
Copy link
Contributor

@evgenykuzyakov evgenykuzyakov commented Apr 30, 2021

  • Expose cur_block and genesis_config from RuntimeStandalone. This allows to manipulate block time, e.g. set to a given timestamp.
  • Use RuntimeConfig::from_protocol_version that fixes storage costs issue. Fixes: near-sdk-sim calculates storage incorrectly  #388
  • Set root account balance to one billion tokens.

Test plan:

  • Modify FT test to verify storage cost fix by limiting initial deposit to 5 NEAR. It is not enough with old storage cost (50kb) and enough with new storage cost (500kb)

@@ -268,7 +271,7 @@ impl RuntimeStandalone {
random_seed: Default::default(),
epoch_id: EpochId::default(),
current_protocol_version: PROTOCOL_VERSION,
config: Arc::from(self.genesis.runtime_config.clone()),
config: RuntimeConfig::from_protocol_version(&self.runtime_config, PROTOCOL_VERSION),
Copy link
Contributor

Choose a reason for hiding this comment

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

The thing with lazy_static that happens inside RuntimeConfig::from_protocol_version feels a bit odd. Seems like tests might start with different genesiss_config, and then config of one test will override config of the other?

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 I think that's exactly the reason why near/nearcore#4263 fails

Copy link
Contributor

Choose a reason for hiding this comment

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

Added a fix for that to near/nearcore#4263 (not that we can immediately use that in the SDK)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was mostly for mainnet upgrade. I guess we can also override Default for GenesisConfig to use this instead of applying it in the produce block. This way the user can override the config if needed.

Comment on lines 149 to 160
pub genesis: GenesisConfig,
tx_pool: TransactionPool,
transactions: HashMap<CryptoHash, SignedTransaction>,
outcomes: HashMap<CryptoHash, ExecutionOutcome>,
profile: HashMap<CryptoHash, ProfileData>,
cur_block: Block,
pub cur_block: Block,
runtime: Runtime,
tries: ShardTries,
pending_receipts: Vec<Receipt>,
epoch_info_provider: Box<dyn EpochInfoProvider>,
pub last_outcomes: Vec<CryptoHash>,
pub runtime_config: Arc<RuntimeConfig>,
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity, what is the need for these three to be public now? Would you mind linking to where they are used externally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is for tests only, it worth give more control to the user. cur_block should be exposed as is and there is no reason to hide it from the user. The main idea is you should have granular control over block time. For example when testing lockup contracts we need to check what would be the balance after a year. For staking pool we want to have access to epoch ID. But if we rely only on produce block advancing this variables, it's hard to write assert statements because you either should replicate math in tests, or just hardcode the output without verifying math. But if you have precise time control, then you can just use round numbers.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I was just curious what the interactions would look like to explore if there is an alternative that's easier to maintain/use

Copy link
Contributor

@mikedotexe mikedotexe left a comment

Choose a reason for hiding this comment

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

This is perfect timing for something I'm helping with on the weekends.

@evgenykuzyakov evgenykuzyakov merged commit 0507deb into master May 3, 2021
@evgenykuzyakov evgenykuzyakov deleted the expose-cur-block branch May 3, 2021 19:10
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.

near-sdk-sim calculates storage incorrectly
5 participants