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.

Sign up for GitHub

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

Add function_arguments_typed to return alloy Vec<DynSolType> #4

Closed
wants to merge 8 commits into from
Closed

Add function_arguments_typed to return alloy Vec<DynSolType> #4

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
833b972
Add function_arguments_typed to return alloy DynSolType
wtdcode Jul 4, 2024
9269436
Fix string
wtdcode Jul 4, 2024
e46677d
Fix crash when copy out of range code
wtdcode Jul 5, 2024
88ba561
Format
wtdcode Jul 5, 2024
ce71038
Merge master branch and solve reviews
wtdcode Aug 24, 2024
8a95b21
Minor fixes
wtdcode Aug 24, 2024
c70e039
Format code
wtdcode Aug 24, 2024
05d4368
Add test for previous crash cases
wtdcode Aug 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions rust/src/evm/vm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,9 +368,27 @@ where
Err(UnsupportedOpError { op }.into())
} else {
let mut data: Vec<u8> = vec![0; size];
let code_len = self.code.len();

let (from_code_length, _) = if src_off + size < code_len {
Copy link
Owner

Choose a reason for hiding this comment

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

  1. Why are you returning two values from this if, but always ignoring the second one? Could it be simplified to just the 1st value?
  2. I think a small change to the original code fits better here:
let code_len = self.code.len();
if src_off < code_len {
    let n = std::cmp::min(size, code_len - src_off);
    data[0..n].copy_from_slice(&self.code[src_off..src_off + n]);
}

wdyt?

Copy link
Contributor Author

@wtdcode wtdcode Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  1. I can't remember exactly the reason because I was fixing this in a midnight to solve evmole crashing with a random unverified contract. Anyway, you are right, it can be simplified.
  2. This one is shorter but harder to understand and maintain. I would like to leave it clearly and explicitly as current implementation so that the thing in point 1 above won't happen next time =). I can also leave a few more comments about the rationale of CODECOPY.

Copy link
Owner

Choose a reason for hiding this comment

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

Let's review it again after (1) simplification; it will probably really be much easier to read and understand

Copy link
Owner

Choose a reason for hiding this comment

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

I've pushed if src_off < code_len { to the master branch just to make master work while we are here. Feel free to rewrite it according to your proposal if you want.

(size, 0)
} else if src_off < code_len {
// | <---- code ----> |
// | <------ CODECOPY ------> |
// src_off
// | from_code | from_zero |
(code_len - src_off, src_off + size - code_len)
} else {
(0, size)
};

if from_code_length != 0 {
data[0..from_code_length]
.copy_from_slice(&self.code[src_off..src_off + from_code_length]);
}

// No need to deal with from_zero because they are already zero

let n = std::cmp::min(size, self.code.len() - src_off);
data[0..n].copy_from_slice(&self.code[src_off..src_off + n]);
self.memory.store(mem_off, data, None);
Ok(StepResult::new(op, 3))
}
Expand Down