feat: add batch precompile #989
Conversation
precompiles/utils/src/data.rs
Outdated
| /// Move the reading cursor with provided length, and return a range from the previous cursor | ||
| /// location to the new one. | ||
| /// Checks cursor overflows. | ||
| fn move_cursor(&mut self, len: usize) -> EvmResult<Range<usize>> { | ||
| pub fn move_cursor(&mut self, len: usize) -> EvmResult<Range<usize>> { |
There was a problem hiding this comment.
This shouldn't be public - we discussed it either here already or in another PR.
There was a problem hiding this comment.
This is the exact code that was used from moonbeam.
they don't have to make it public because they have defined this function in mod.rs which makes this function public inside a module so the don't have to explicitly make it public which is not the case for us.
The best we can do is to make a wrapper around this function but then we will have to make that public so it will be the same thing, if you have a better way then kindly suggest.
There was a problem hiding this comment.
I think we also discussed the nature of this issue before.
This is why you added pub fn get_input_from_range, right?
If you need a functionality which is not available in the existing interface, the answer is not to just make the encapsulated internals public. That's bad design.
Since pub fn get_input_from_range is not good enough for your needs, adapt it so it is.
Since I took a more detailed look now, isn't the existing fn read_raw_bytes exactly what you need?
pub fn read_raw_bytes(&mut self, len: usize) -> EvmResult<&[u8]> {
let range = self.move_cursor(len)?;
let data = self
.input
.get(range)
.ok_or_else(|| revert("tried to parse raw bytes out of bounds"))?;
Ok(data)
}compared to yours:
/// Return Option<&[u8]> from a given range for EvmDataReader
pub fn get_input_from_range(&self, range: Range<usize>) -> Option<&[u8]> {
self.input.get(range)
}
...
impl<K: Kind, S: Get<u32>> EvmData for BoundedBytesString<K, S> {
fn read(reader: &mut EvmDataReader) -> EvmResult<Self> {
...
// Get valid range over the bytes data.
let range = inner_reader.move_cursor(array_size)?;
let data = inner_reader
.get_input_from_range(range)
.ok_or_else(|| revert(K::signature()))?;There was a problem hiding this comment.
Yes, you are right, we can use this function but I would still argue that this just happens to be a wrapper function that is already public and doesn't add to the improvement of design.
But we can have discussion over this later, I have changed this as per your suggestion.
There was a problem hiding this comment.
The difference is that move_cursor manipulates internal fields/attributes.
read_raw_bytes is a higher level function that performs a more complex operation, and hides implementation details.
Maybe not the best read, but first thing Google throws out:
https://doc.rust-lang.org/book/ch17-01-what-is-oo.html#encapsulation-that-hides-implementation-details
The less functionality you expose to the outside, the better.
The higher level the exposed functionality is, the better.
Plus, all the stuff mentioned before (changing existing private to public).
Internet is full of resources to read up about this.
precompiles/utils/src/data.rs
Outdated
| @@ -285,7 +290,7 @@ impl EvmDataWriter { | |||
|
|
|||
| /// Write arbitrary bytes. | |||
| /// Doesn't handle any alignement checks, prefer using `write` instead if possible. | |||
| fn write_raw_bytes(mut self, value: &[u8]) -> Self { | |||
| pub fn write_raw_bytes(mut self, value: &[u8]) -> Self { | |||
precompiles/utils/src/testing/mod.rs
Outdated
There was a problem hiding this comment.
The testing module brings a lot of new features - should the existing precompiles be updated to use them in their tests?
(not as part of this PR of course)
Or is this just the split of old testing.rs?
There was a problem hiding this comment.
it brings some new features but it is backward compatible.
| } | ||
| } | ||
|
|
||
| Ok(succeed(EvmDataWriter::new().write(true).build())) |
There was a problem hiding this comment.
God function! 🙂
I understand it's code reuse, but try checking the UT coverage, just in case.
There are a lot of branches to account for.
There was a problem hiding this comment.
It has been designed carefully, we only allow it to pass for the function like BatchSome or BatchSomeUntilFailure and not for BatchAll.
There was a problem hiding this comment.
I'm sure it was but still, it's an incredibly complex function - is it fully covered by UTs?
You can use tarpaulin tool to check this: cargo tarpaulin -o html
(it's not a perfect tool but gives good enough picture)
We are just reusing this, we hadn't really written it from scratch. It's easy to overlook something when just reading/reusing the code.
There was a problem hiding this comment.
I ran tarpaulin and I think it is a good score.
precompiles/batch/src/lib.rs: 127/143
I checked all the tests again, we have enough tests imo, MB has done good testing.
There was a problem hiding this comment.
I wouldn't know to say whether that's a good score or not.
But checking coverage in %, and even better, taking a look at the output file, gives a clearer picture if some important function/branch was missed.
gitofdeepanshu
dismissed stale reviews from ashutoshvarma and Dinonard
via
c96c10e
Minimum allowed line rate is |
* feat: add batch precompile * typo * add tests * minor fixes and updates * refactor, add batch to local * fix expect_arguments * add logs * update license * taplo fix * more refactor, remote evm crate * remove evm crate, update utils version, add std in dappsstaking * update pallet_balances::config in mock * update pallet_evm config * update license * add to std-feature * utils: rectify patch to minor update * remove pub from logs function * fix typos * remove comments * refactor * fmt
Pull Request Summary
Batch Precompile
The Batch precompile allows users to combine multiple calls into a single transaction. These calls are executed on behalf of the precompile caller. The caller provides a list of addresses to call, along with corresponding values and call data. If the values and call data arrays are shorter than the list of addresses, default values (0 and empty data) are used for the missing indices.
There are three available functions to determine how the precompile behaves in case of call failures:
BatchSome: If a subcall fails, it reverts that specific subcall but continues to execute any remaining calls in the batch.
BatchSomeUntilFailure: If a subcall fails, it reverts that specific subcall and stops executing any following subcalls. However, the precompile itself does not revert.
BatchAll: If any subcall fails, the precompile reverts itself, undoing any previous successful calls in the batch as well.
Check list