std::evm functions should take buffer types #891 (Part 1) #895
Conversation
Fixed call functions of the ethereum#891 issue.
return_mem, revert_mem, log, keccak256_mem, create
fixed comments from ethereum#893
saifalkatout
changed the title
crates/library/std/src/context.fe
Outdated
| output_offset: buf.offset(), | ||
| output_len: buf.output_len() | ||
| ) == 1 | ||
| buffer:buf |
There was a problem hiding this comment.
I think still think we should change all buffer identifiers to buf, it looks nice and conventional imo.
this can simply be written as buf instead of buffer: buf
There was a problem hiding this comment.
@saifalkatout do you feel that the identifier should be buffer instead of buf?
There was a problem hiding this comment.
No, I think buffer is better since it will be written more neatly, I guess this is an outdated version ?
There was a problem hiding this comment.
yes, the specific code line we are commenting on is outdated. I see that all identifiers have been changed to buffer
if we change all of the parameter names to buf, then we can use buf everywhere, so we wont have to write buffer: buf or buf: buffer anywhere. it can all just be buf
There was a problem hiding this comment.
I guess stick with buffer on everything because it is more easily understood, my only reservation would be that it can be also easily understood using buf. So the extra characters would be meaningless.
There was a problem hiding this comment.
buf is the conventional name for buffer instances and I think we should stick with that
a couple examples:
rust: https://docs.rs/bytes/latest/bytes/buf/trait.Buf.html
java: https://docs.oracle.com/javase/8/docs/api/java/nio/Buffer.html
|
Sure thing, are we going to incrementally change it ?? so whenever we do sth in a function that has buffer arg. Or do we just do all of them later and in a separate PR for the whole std library ? |
there were no |
|
oh, wait. are you getting a naming error? something like |
this is something Yoshi will have fixed in hist v2 branch. for the time being we can just import the buffer by themselves
|
Snap files for changes on PR ethereum#891
|
Snap files added, I still need to do the |
used the curly bracket import, changed the name of the argument to buf
|
Changed the argument name to buf. |
| } | ||
|
|
||
| pub unsafe fn log1(offset p: u256, len s: u256, topic1 t1: u256) { | ||
| return __log1(p, s, t1) | ||
| pub fn log1(buf: MemoryBuffer, topic1 t1: u256) { |
Implement CallDataReader, as per the tasks of issue ethereum#900
buf changes from master repo
Merge pull request ethereum#895 from saifalkatout/master
What was wrong?
A number of functions defined in std::evm take offsets and lengths for regions of memory and calldata. These should take buffer types instead.
How was it fixed?
updated revert_mem, return_mem, ext_code_copy, keccak256_mem, create, create2, call, and log
To-Do
-Finish the rest of the functions in part 2