Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Turn storage items into parameters #2883

Merged
merged 32 commits into from
Jul 2, 2019
Merged

Conversation

dvc94ch
Copy link
Contributor

@dvc94ch dvc94ch commented Jun 17, 2019

Closes #2400

srml/contract/src/gas.rs Outdated Show resolved Hide resolved
srml/contract/src/gas.rs Outdated Show resolved Hide resolved
@dvc94ch
Copy link
Contributor Author

dvc94ch commented Jun 17, 2019

There is something wrong with the executor tests with the error Transaction tree root must be valid. Any idea what's going on here?

@dvc94ch dvc94ch added A0-please_review Pull request needs code review. I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. and removed A4-gotissues labels Jun 17, 2019
@bkchr
Copy link
Member

bkchr commented Jun 17, 2019

If you change a test in such a way that you would expect that the trie-root changes, then this is okay and you just need to use these new values as the expected ones.

srml/balances/src/mock.rs Show resolved Hide resolved
srml/contract/src/lib.rs Outdated Show resolved Hide resolved
@dvc94ch
Copy link
Contributor Author

dvc94ch commented Jun 18, 2019

So the problem with the integration test is that the local testnet now has the same values as the normal net. This means bob doesn't have enough funds to transfer. Who can send bob some more funds? :)

The test generates 512 identical blocks, so what looked like syncing to a net was not actually syncing to a net, and that also explains why there wheren't enough funds in the genesis block.

@dvc94ch dvc94ch force-pushed the dvc-const-params branch 3 times, most recently from d4884f7 to 209dc3f Compare June 18, 2019 17:36
@jacogr
Copy link
Contributor

jacogr commented Jun 19, 2019

Yes, it would seem the dev net pulls in the same default config, i.e. did some democracy tests yesterday on a dev network and ended up tweaking these to get some saner defaults -

parameter_types! {
	pub const LaunchPeriod: BlockNumber = 5 * MINUTES;
	pub const VotingPeriod: BlockNumber = 5 * MINUTES;
	pub const EmergencyVotingPeriod: BlockNumber = 5 * MINUTES;
	pub const MinimumDeposit: Balance = 1000;
	pub const EnactmentPeriod: BlockNumber = 5 * MINUTES;
	pub const CooloffPeriod: BlockNumber = 5 * MINUTES;
}

So previously the dev network genesis overwrote with "nice for dev values", so not 100% sure where that happens atm. However, it would be "very useful" to have the dev/test with different values to the normal networks.

@dvc94ch dvc94ch force-pushed the dvc-const-params branch 2 times, most recently from 5590912 to 7e77d92 Compare June 19, 2019 07:10
@dvc94ch dvc94ch requested a review from pepyakin June 21, 2019 15:13
@dvc94ch
Copy link
Contributor Author

dvc94ch commented Jun 21, 2019

So some items like the gas price can change as frequently as every block, although there isn't any code yet there to do so. If there are other items that should remain in storage and not be parameters, please point them out to me.

node/executor/src/lib.rs Outdated Show resolved Hide resolved
@gavofyork gavofyork added A5-grumble and removed A0-please_review Pull request needs code review. I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. labels Jun 21, 2019
Copy link
Contributor

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

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

Regarding contracts, I think block_gas_limit might change relatively frequently.
However, I am not sure if I am confident in the initial values of these parameters so as a note any of these values might need some tuning.

@gavofyork
Copy link
Member

@bkchr this doesn't allow for module-local consts to make it into the metadata though...

@gavofyork
Copy link
Member

(though i don't really care; whatever can be in fastest for now, i guess.)

@bkchr
Copy link
Member

bkchr commented Jun 27, 2019

After talking to @gavofyork, I will do the metadata stuff today. After that is merged we can merge this one.

I'm still in favor of using associated constants. Looking into the code, a lot of repetition is happening and this will happen again in Polkadot and each other Blockchain building on Substrate.

@gavofyork
Copy link
Member

yeah, happy to see a switch to associated constants.

@dvc94ch dvc94ch force-pushed the dvc-const-params branch 2 times, most recently from b5bfafa to a2a1372 Compare July 1, 2019 15:48
@dvc94ch dvc94ch added A0-please_review Pull request needs code review. and removed A1-onice labels Jul 1, 2019
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Overall looking good.

Just some minor stuff + some of my last comments.

Maybe we could put the default parameter type values into each module?

node/runtime/src/lib.rs Outdated Show resolved Hide resolved
node/runtime/src/lib.rs Outdated Show resolved Hide resolved
@dvc94ch dvc94ch added A5-grumble and removed A0-please_review Pull request needs code review. labels Jul 1, 2019
@dvc94ch dvc94ch merged commit f835f9f into paritytech:master Jul 2, 2019
niklasad1 pushed a commit that referenced this pull request Jul 4, 2019
* balances: Turn storage items into parameters.

* contract: Turn storage items into parameters.

* council: Turn storage items into parameters.

* finality-tracker: Turn storage items into parameters.

* treasury: Turn storage items into parameters.

* democracy: Fix tests.

* example: Fix tests.

* executive: Fix tests.

* staking: Fix tests.

* Update runtime.

* Update template-node.

* Update runtime version.

* Fix executor tests.

* Fix node cli tests.

* Address grumbles.

* Add removed default values to docs.

* Make gas price a storage item.

* Set associated consts must be callable outside of build.

* Fix not enough gas to pay for transfer fee.

* Fix build.

* Emit metadata.

* Fix build.

* Add default values for all parameter types.

* Fix build.

* Fix build.

* Fix build.

* Fix build.
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Jul 10, 2019
* balances: Turn storage items into parameters.

* contract: Turn storage items into parameters.

* council: Turn storage items into parameters.

* finality-tracker: Turn storage items into parameters.

* treasury: Turn storage items into parameters.

* democracy: Fix tests.

* example: Fix tests.

* executive: Fix tests.

* staking: Fix tests.

* Update runtime.

* Update template-node.

* Update runtime version.

* Fix executor tests.

* Fix node cli tests.

* Address grumbles.

* Add removed default values to docs.

* Make gas price a storage item.

* Set associated consts must be callable outside of build.

* Fix not enough gas to pay for transfer fee.

* Fix build.

* Emit metadata.

* Fix build.

* Add default values for all parameter types.

* Fix build.

* Fix build.

* Fix build.

* Fix build.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Constify parameter value storage items
7 participants