refac: Use offset from SDK for executable loaders #6693
Conversation
|
|
|
Hi @kayagokalp @Br1ght0ne @Salka1988 @hal3e please can you review? |
| let loader_data_section_offset = loader.data_offset_in_code(); | ||
| if let ProgramABI::Fuel(mut fuel_abi) = pkg.program_abi.clone() { | ||
| println_action_green("Generating", "loader abi for the uploaded executable."); | ||
| let json_abi_path = out_dir.join(format!("{pkg_name}-loader-abi.json")); | ||
| let original_data_section = extract_data_offset(&pkg.bytecode.bytes).unwrap(); | ||
| let offset_shift = original_data_section - loader_data_section_offset; | ||
| // if there are configurables in the abi we need to shift them by `offset_shift`. | ||
| let configurables = fuel_abi.configurables.clone().map(|configs| { | ||
| configs | ||
| .into_iter() | ||
| .map(|config| Configurable { | ||
| offset: config.offset - offset_shift as u64, | ||
| ..config.clone() | ||
| }) | ||
| .collect() | ||
| }); | ||
| fuel_abi.configurables = configurables; | ||
| let json_string = serde_json::to_string_pretty(&fuel_abi)?; | ||
| std::fs::write(json_abi_path, json_string)?; |
There was a problem hiding this comment.
Looks good to me, but I believe we need a test for this. Especially because we are not sure what is going to happen if the binary does not have a data section. In earlier implementation it was obvious that the function to calculate offset returns None in that case but now I am not sure what is going to happen.
So if you are up for it, let's add a test which get's its oracle values (like the calculated shifted values with the previous implementation) and in a next commit let's apply this change and see if the calculated values are same.
There was a problem hiding this comment.
Hi @kayagokalp I did look at the SDK before I implemented this change, the function implementation is guaranteed to always return an offset, and cases where it wouldn't the program crashes here's a snippet from Executable<Loader> impl block
pub fn data_offset_in_code(&self) -> usize {
self.code_with_offset().1
}
fn code_with_offset(&self) -> (Vec<u8>, usize) {
let mut code = self.state.code.clone();
self.state.configurables.update_constants_in(&mut code);
let blob_id = self.blob().id();
transform_into_configurable_loader(code, &blob_id)
.expect("checked before turning into a Executable<Loader>")
}But if you'd want a test after looking at this, what should I be testing for? that the value isn't zero? within a specific range? I'd like to hear your POV
There was a problem hiding this comment.
Hey again, it is possible that a binary does not contain any data section, that happens if the sway code does not have any constants and no configurable section. So if SDK is expecting this to be checked before calling the function, we might need to check this explicitly.
For testing we can use a example test script or predicate's binary from /test/data. And (possibly) extract the shift calculation to a separate function so that we can write tests against it.
Basically call this new calculate shift function and check if the calculated shift is being calculated correctly for two types of sway scripts:
- A script that has a single main function and nothing else.
- The script like https://github.com/FuelLabs/sway/blob/master/forc-plugins/forc-client/test/data/deployed_script/src/main.sw which needs this shift.
I can help with testing if you need it feel free to ping me
There was a problem hiding this comment.
Yeah, I'd need some help with testing it, How do I ping you? I tried on twitter, 2 days ago and you haven't responded
There was a problem hiding this comment.
I will be adding the tests to this PR directly to better streamline the process, possibly tomorrow. Other than the tests this looks good to me, after the test I'll approve and hunt for your second approval :) Thanks again for the contribution.
Description
Uses the offset exposed by the SDK instead of recalculating it and having to manually update the code each time the implementation changes.
closes #6683
Checklist
Breaking*orNew Featurelabels where relevant.