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

Jump to bottom

Use offset from SDK for executable loaders instead of re-calculating it #6683

Open
kayagokalp opened this issue Nov 1, 2024 · 3 comments · May be fixed by #6693
Open

Use offset from SDK for executable loaders instead of re-calculating it #6683

kayagokalp opened this issue Nov 1, 2024 · 3 comments · May be fixed by #6693
Assignees
@Abeeujah
Labels
code quality forc-deploy Everything to do with forc-deploy good first issue Good for newcomers

Comments

@kayagokalp
Copy link
Member

Description

Loader offset is exposed by sdk as of FuelLabs/fuels-rs#1522 currently we are recalculating this value in forc-deploy because at the time of the implementation this was not exposed by sdk. This is bad because if anything changes around that area, we need to update it manually. We should use the exposed value from sdk instead of recalculating.

FuelLabs/fuels-rs#1522

Context

sway/forc-plugins/forc-client/src/op/deploy.rs

Lines 357 to 360 in b6bbbf8

/// Calculates the loader data offset. Returns a `None` if the original `binary`
/// does not have a data section (no configurables and no args). Otherwise
/// returns the new offset of the data section.
fn loader_data_offset(binary: &[u8], blob_id: &BlobId) -> Result<Option<usize>> {

Solution

Basically remove the function showed in context, and get the exposed value from SDK directly.
@kayagokalp kayagokalp added good first issue Good for newcomers code quality forc-deploy Everything to do with forc-deploy labels Nov 1, 2024
@Abeeujah
Copy link

Abeeujah commented Nov 2, 2024

Hi, this sounds great, I'd love to jump on this!

@kayagokalp
Copy link
Member Author

Hi, this sounds great, I'd love to jump on this!

Hey @Abeeujah go for it! Let me know if you need any help/guidance.

@Abeeujah
Copy link

Abeeujah commented Nov 4, 2024 •
edited
Loading

Hi @kayagokalp going through the sdk, in the impl block for Executable<Loader> I found the function being exposed to get the offset data_offset_in_code I've updated deploy.rs to use that instead and removed the duplicate function recalculating the value.

let loader_data_section_offset = loader.data_offset_in_code();

@Abeeujah Abeeujah linked a pull request Nov 4, 2024 that will close this issue
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Abeeujah Abeeujah

Labels
code quality forc-deploy Everything to do with forc-deploy good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refac: Use offset from SDK for executable loaders Abeeujah/sway
2 participants
@kayagokalp @Abeeujah