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

Pre v0.6 dependency updates #215

Merged
merged 69 commits into from
Feb 9, 2021
Merged

Conversation

notlesh
Copy link
Contributor

@notlesh notlesh commented Jan 29, 2021

What does it do?

Updates frontier, substrate, cumulus, polkadot.

This updates:

This PR should be in good shape now, but it depends on some upstream PRs:

Also:

  • Fix logging init code (see comments)
  • CI green

sc_cli::init_logger(InitLoggerParams {
tracing_receiver: sc_tracing::TracingReceiver::Log,
..Default::default()
})?;
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to properly fix this (and same issue 20 lines lower)

Copy link
Contributor

Choose a reason for hiding this comment

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

node/parachain/Cargo.toml Outdated Show resolved Hide resolved
@@ -257,11 +257,11 @@ pub const WEIGHT_PER_GAS: u64 = WEIGHT_PER_SECOND / GAS_PER_SECOND;
pub struct MoonbeamGasWeightMapping;

impl pallet_evm::GasWeightMapping for MoonbeamGasWeightMapping {
fn gas_to_weight(gas: usize) -> Weight {
fn gas_to_weight(gas: u64) -> Weight {
Weight::try_from((gas as u64).saturating_mul(WEIGHT_PER_GAS)).unwrap_or(Weight::MAX)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah finally a correct type for gas.
You can remove the as 64 here and maybe most of the rest too as both the Weight and the gas are 64 I think

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 kept u32::MAX as u64 in hopes of being consistent with our previous fallback. Is this ever useful, anyway, and would u64::MAX somehow be better? (or were you merely suggesting that 'as u64' was redundant?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Weight::try_from((gas as u64).saturating_mul(WEIGHT_PER_GAS)).unwrap_or(Weight::MAX)
gas.saturating_mul(WEIGHT_PER_GAS)

Does this do the same thing? I think so.

runtime/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@crystalin crystalin left a comment

Choose a reason for hiding this comment

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

Good work.
Left few comments

notlesh and others added 2 commits February 2, 2021 21:26
Co-authored-by: Joshy Orndorff <JoshOrndorff@users.noreply.github.com>
@tgmichel
Copy link
Contributor

tgmichel commented Feb 3, 2021

Would be great to have frontier's 52cfbc4e88ad5c36db68891f20a6540613b81141 on this iteration, as it solves some The Graph requirements and we can remove the skipped test at 7890e51.

Copy link
Contributor

@JoshOrndorff JoshOrndorff left a comment

Choose a reason for hiding this comment

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

Looks awesome! Made a few little comments.

Feedback on the Frontier part:

  • I like the branch name v0.6-moonbeam
  • Thanks for de-duping parity-scale-codec
  • The commit history is so nice and clean. I like the idea of keeping all the changes that we made to stock frontier right at the tip.
  • I don't think 2246428858 is necessary.
  • @tgmichel could you take a look at https://github.com/purestake/frontier/commits/v0.6-moonbeam and confirm it has all the important stuff? nvm, you just did :)

runtime/src/lib.rs Outdated Show resolved Hide resolved
sc_cli::init_logger(InitLoggerParams {
tracing_receiver: sc_tracing::TracingReceiver::Log,
..Default::default()
})?;
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

runtime/precompiles/src/lib.rs Show resolved Hide resolved
@@ -257,11 +257,11 @@ pub const WEIGHT_PER_GAS: u64 = WEIGHT_PER_SECOND / GAS_PER_SECOND;
pub struct MoonbeamGasWeightMapping;

impl pallet_evm::GasWeightMapping for MoonbeamGasWeightMapping {
fn gas_to_weight(gas: usize) -> Weight {
fn gas_to_weight(gas: u64) -> Weight {
Weight::try_from((gas as u64).saturating_mul(WEIGHT_PER_GAS)).unwrap_or(Weight::MAX)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Weight::try_from((gas as u64).saturating_mul(WEIGHT_PER_GAS)).unwrap_or(Weight::MAX)
gas.saturating_mul(WEIGHT_PER_GAS)

Does this do the same thing? I think so.

fn weight_to_gas(weight: Weight) -> usize {
usize::try_from(weight.wrapping_div(WEIGHT_PER_GAS)).unwrap_or(usize::MAX)
fn weight_to_gas(weight: Weight) -> u64 {
u64::try_from(weight.wrapping_div(WEIGHT_PER_GAS)).unwrap_or(u32::MAX as u64)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
u64::try_from(weight.wrapping_div(WEIGHT_PER_GAS)).unwrap_or(u32::MAX as u64)
weight.wrapping_div(WEIGHT_PER_GAS)

Both input values are u64, so the result will certainly be a u64. The try_from cannot possibly fail.

runtime/src/lib.rs Outdated Show resolved Hide resolved
runtime/src/parachain.rs Outdated Show resolved Hide resolved
@tgmichel
Copy link
Contributor

tgmichel commented Feb 3, 2021

We are missing stuff related to the new EthFilterApi. An example is available in frontier's template:

We need to extend the rpc server:
https://github.com/PureStake/frontier/blob/v0.6-moonbeam/template/node/src/rpc.rs#L118

Update the service, for example:
https://github.com/PureStake/frontier/blob/v0.6-moonbeam/template/node/src/service.rs#L99
https://github.com/PureStake/frontier/blob/v0.6-moonbeam/template/node/src/service.rs#L123

And setup the maintenance task:
https://github.com/PureStake/frontier/blob/v0.6-moonbeam/template/node/src/service.rs#L237

@notlesh I can do this in this same branch if you want.
Edit: @notlesh I will also check our v0.6 frontier branch, probably we want to use the StorageProvider instead the runtime api in this new Rpc module.

@tgmichel
Copy link
Contributor

tgmichel commented Feb 3, 2021

Also we want to include the tests for the EthFilterApi in:
https://github.com/PureStake/frontier/blob/v0.6-moonbeam/ts-tests/tests/test-filter-api.ts

expect(context.polkadotApi.events.ethereum.Executed.is(events[2])).to.be.true;
expect(context.polkadotApi.events.system.ExtrinsicSuccess.is(events[3])).to.be.true;
// TODO: what event was inserted here?
expect(context.polkadotApi.events.ethereum.Executed.is(events[3])).to.be.true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joelamouche Is this obvious to you, or do you have some documentation handy that you could share?

@notlesh notlesh merged commit 5308f49 into master Feb 9, 2021
@notlesh notlesh deleted the notlesh-pre-v0.6-dependency-updates branch February 9, 2021 21:05
/// Given the 500ms Weight, from which 75% only are used for transactions,
/// the total EVM execution gas limit is: GAS_PER_SECOND * 0.500 * 0.75 => 6_000_000.
pub const GAS_PER_SECOND: u64 = 16_000_000;
/// Given the 500ms Weight, from which 65% only are used for transactions,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@notlesh This was correct before.
A block has 75% dedicated to tx.
65% is for the max of a single transaction

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.

5 participants