-
Notifications
You must be signed in to change notification settings - Fork 481
[core] add gas arg to ext_gas_price #461
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
Changes from all commits
ff57bb1
3f829b1
f9d56d1
9b5c99c
6a88353
c1c22b4
39ede31
787aa15
d216d11
a6668b7
d8a7e98
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,15 +14,21 @@ | |
|
|
||
| //! Primitive traits for runtime arithmetic, copied from substrate | ||
|
|
||
| use core::ops::{ | ||
| Add, | ||
| AddAssign, | ||
| Div, | ||
| DivAssign, | ||
| Mul, | ||
| MulAssign, | ||
| Sub, | ||
| SubAssign, | ||
| use core::{ | ||
| convert::{ | ||
| TryFrom, | ||
| TryInto, | ||
| }, | ||
| ops::{ | ||
| Add, | ||
| AddAssign, | ||
| Div, | ||
| DivAssign, | ||
| Mul, | ||
| MulAssign, | ||
| Sub, | ||
| SubAssign, | ||
| }, | ||
| }; | ||
| use num_traits::{ | ||
| checked_pow, | ||
|
|
@@ -40,7 +46,7 @@ use num_traits::{ | |
| /// if needed. | ||
| pub trait BaseArithmetic: | ||
| Sized | ||
| + From<u32> | ||
| + From<u8> | ||
| + Bounded | ||
| + Ord | ||
| + PartialOrd<Self> | ||
|
|
@@ -57,21 +63,19 @@ pub trait BaseArithmetic: | |
| + DivAssign<Self> | ||
| + CheckedMul | ||
| + Saturating | ||
| + TryFrom<u16> | ||
| + TryFrom<u32> | ||
| + TryFrom<u64> | ||
| + TryFrom<u128> | ||
| + TryFrom<usize> | ||
| + TryInto<u16> | ||
| + TryInto<u32> | ||
| + TryInto<u64> | ||
| + TryInto<u128> | ||
| + TryInto<usize> | ||
| // Further trait bounds from the original BaseArithmetic trait | ||
| // that we could use to extend ink!'s BaseArithmetic trait. | ||
| // | ||
| // From<u8> + | ||
| // From<u16> + | ||
| // From<u32> + | ||
| // TryFrom<u64> + | ||
| // TryFrom<u128> + | ||
| // TryFrom<usize> + | ||
| // TryInto<u8> + | ||
| // TryInto<u16> + | ||
| // TryInto<u32> + | ||
| // TryInto<u64> + | ||
| // TryInto<u128> + | ||
| // TryInto<usize> + | ||
| // UniqueSaturatedInto<u8> + | ||
| // UniqueSaturatedInto<u16> + | ||
| // UniqueSaturatedInto<u32> + | ||
|
|
@@ -92,7 +96,7 @@ pub trait BaseArithmetic: | |
|
|
||
| impl<T> BaseArithmetic for T where | ||
| T: Sized | ||
| + From<u32> | ||
| + From<u8> | ||
| + Bounded | ||
| + Ord | ||
| + PartialOrd<Self> | ||
|
|
@@ -108,6 +112,16 @@ impl<T> BaseArithmetic for T where | |
| + DivAssign<Self> | ||
| + CheckedMul | ||
| + Saturating | ||
| + TryFrom<u16> | ||
| + TryFrom<u32> | ||
| + TryFrom<u64> | ||
| + TryFrom<u128> | ||
| + TryFrom<usize> | ||
| + TryInto<u16> | ||
| + TryInto<u32> | ||
| + TryInto<u64> | ||
| + TryInto<u128> | ||
| + TryInto<usize> | ||
|
Comment on lines
+115
to
+124
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain why we need this for this PR?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this diverging from what Substrate bounds its types?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The
It's converging, I thought I would add the other |
||
| { | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,8 @@ use crate::env::{ | |
| TypedEnv, | ||
| }; | ||
| use ink_primitives::Key; | ||
| use core::convert::TryInto; | ||
| use num_traits::Bounded; | ||
|
|
||
| impl EnvInstance { | ||
| /// Returns the callee account. | ||
|
|
@@ -153,11 +155,15 @@ impl TypedEnv for EnvInstance { | |
| .map_err(Into::into) | ||
| } | ||
|
|
||
| fn gas_price<T: EnvTypes>(&mut self) -> Result<T::Balance> { | ||
| self.chain_spec | ||
| /// Emulates gas price calculation | ||
| fn gas_price<T: EnvTypes>(&mut self, gas: u64) -> Result<T::Balance> { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice to have a test for this function somewhere, since the logic now got more complex and logic surrounding cost analysis should be considered criticial imho. Besides this the code looks sound to me.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it worth adding tests for the offchain environment?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Theoretically yes they are worth. However, the off-chain environment currently lacks tests all over the place because originally it was not intended to stay for long since at the time of writing some standalone Seal implementation was still deemed to be realistic. Consider it a hack that came to stay.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a test 👇 |
||
| use crate::env::arithmetic::Saturating as _; | ||
|
|
||
| let gas_price = self.chain_spec | ||
| .gas_price::<T>() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be cool if there was an actual conversion config for the off-chain environment so that the returned amount at least scales linearly to the input. |
||
| .map_err(|_| scale::Error::from("could not decode gas price")) | ||
| .map_err(Into::into) | ||
| .map_err(|_| scale::Error::from("could not decode gas price"))?; | ||
|
|
||
| Ok(gas_price.saturating_mul(gas.try_into().unwrap_or_else(|_| Bounded::max_value()))) | ||
| } | ||
|
|
||
| fn gas_left<T: EnvTypes>(&mut self) -> Result<T::Balance> { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why this change is required for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this brings it into line with the substrate
BaseArithmetic. TheFrom<u32>bound is now defined in theAtLeast32Bittrait, and it is assumed forBaseArtithmetictrait that we are least dealing withu8There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not strictly required for this PR, it's just something I overlooked in #463.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay so am I right that the above changes actually are aligning ink! further with Substrate traits? Just want to be sure about this because I really am not familiar with the Substrate tests but getting traits righti is really really important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly.
We are still missing a few more e.g. the rest of the
Checked*traits andUniqueSaturatedFrom/UniqueSaturatedInto, but these are definitely out of scope for this PR.