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

Compute calldata offset on B256 argument #203

Merged
merged 4 commits into from
Apr 7, 2022

Conversation

digorithm
Copy link
Member

Due to some changes somewhere (VM, compiler? Hard to say because as of now we don't have proper E2E tests and too many fast-moving pieces to pinpoint the actual root cause), contract calls with a single B256 argument stopped working because we're not including the calldata offset in the script_data.

This workaround (the check for calldata offset) will be removed entirely once this issue is done which is currently blocked by this one in the compiler.

However, this current problem (calls with B256 arg) is blocking @nfurfaro on his auth testing work. This PR unblocks him (see the test case I added to this PR, which is now passing).

This is an extremely simple solution: we're now computing calldata offset if the arg is a B256:

fn should_compute_call_data_offset(args: &[Token]) -> bool {
    match args
        .iter()
        .any(|t| matches!(t, Token::Struct(_) | Token::Enum(_) | Token::B256(_)))
    {
        true => true,
        false => args.len() > 1,
    }
}

That's the only significant change introduced by this PR. The rest of this PR is just adding the projects to test that specific case.

@digorithm digorithm added the bug Something isn't working label Apr 5, 2022
@digorithm digorithm self-assigned this Apr 5, 2022
@@ -0,0 +1,50 @@
contract;
Copy link
Contributor

@mohammadfawaz mohammadfawaz Apr 5, 2022

Choose a reason for hiding this comment

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

The fuels-rs repo probably needs this: https://github.com/FuelLabs/sway/blob/master/.gitattributes for Sway syntax highlighting.

@@ -341,7 +341,7 @@ impl Contract {
fn should_compute_call_data_offset(args: &[Token]) -> bool {
match args
.iter()
.any(|t| matches!(t, Token::Struct(_) | Token::Enum(_)))
.any(|t| matches!(t, Token::Struct(_) | Token::Enum(_) | Token::B256(_)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Tuples should probably be on this list as well? Then there's probably the question about strings and arrays.

Maybe it's better if we invert the condition to check for copy types since they are a limited well defined set that will likely never change?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't support tuples:

pub enum Token {
U8(u8),
U16(u16),
U32(u32),
U64(u64),
Bool(bool),
Byte(u8),
B256(Bits256),
Array(Vec<Token>),
String(String),
Struct(Vec<Token>),
Enum(Box<EnumSelector>),
}

Maybe it's better if we invert the condition to check for copy types since they are a limited well defined set that will likely never change?

That's a good idea, but this whole thing will go away very soon, as soon as this is done. So I didn't wanna shake things too much, but I'm not married to this idea. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah of course, you're right... let's keep it as is :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Rodrigo as well

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.

Some nits, otherwise LGTM!

@@ -341,7 +341,7 @@ impl Contract {
fn should_compute_call_data_offset(args: &[Token]) -> bool {
match args
.iter()
.any(|t| matches!(t, Token::Struct(_) | Token::Enum(_)))
.any(|t| matches!(t, Token::Struct(_) | Token::Enum(_) | Token::B256(_)))
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Rodrigo as well

packages/fuels-abigen-macro/tests/harness.rs Outdated Show resolved Hide resolved
@digorithm digorithm requested a review from iqdecay April 7, 2022 18:39
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.

image

@digorithm digorithm merged commit 7fda121 into master Apr 7, 2022
@digorithm digorithm deleted the rodrigo/offset-on-b256-arg branch April 7, 2022 18:59
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