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.

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

Fix multi-argument contract calls #169

Merged
merged 2 commits into from
Mar 25, 2022
Merged

Conversation

digorithm
Copy link
Member

Calls with multiple arguments, for instance

fn get(x: u64, y: u64) -> u64 {
    x
}

Were broken due to how we compute the call data offset. Before, we added this extra offset only if the argument was a custom type. Turns out, if you have more than one argument, you also need that. The only moment you don't need this extra offset is when the method takes only one arg and it's not a custom type.

This PR makes some simple, non-breaking changes to check whether we should or not compute this offset.

@digorithm digorithm self-assigned this Mar 25, 2022
@digorithm digorithm added the bug Something isn't working label Mar 25, 2022
@digorithm
Copy link
Member Author

cc @AlicanC @luizstacio this probably should also be implemented on fuels-ts. Do you mind confirming that?

@luizstacio
Copy link
Member

luizstacio commented Mar 25, 2022

Were broken due to how we compute the call data offset. Before, we added this extra offset only if the argument was a custom type. Turns out, if you have more than one argument, you also need that. The only moment you don't need this extra offset is when the method takes only one arg and it's not a custom type.

I had implemented it on this PR: FuelLabs/fuels-ts#170

Copy link
Contributor

@iqdecay iqdecay left a comment

Choose a reason for hiding this comment

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

One question, otherwise LGTM!

external_contracts: None,
wallet: wallet.clone(),
})
}

// Returns true if the method call takes custom inputs or has more than one argument. This is used to determine whether we need to compute the `call_data_offset`.
fn should_compute_call_data_offset(args: &[Token]) -> bool {
match args.iter().any(|t| matches!(t, Token::Struct(_))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we also check for Enum and in the future Tuples?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes for enums. Tuples might or might not be a thing, we'll see once the time comes.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes!

@digorithm digorithm requested a review from iqdecay March 25, 2022 15:00
Copy link
Contributor

@iqdecay iqdecay left a comment

Choose a reason for hiding this comment

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

nice!

@digorithm digorithm merged commit 15ebec4 into master Mar 25, 2022
@digorithm digorithm deleted the rodrigo/many-args-call-fix branch March 25, 2022 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants