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

abigen: exclude first 3 fields instead of name matching #75

Closed
adlerjohn opened this issue Jan 29, 2022 · 7 comments · Fixed by #83
Closed

abigen: exclude first 3 fields instead of name matching #75

adlerjohn opened this issue Jan 29, 2022 · 7 comments · Fixed by #83
Assignees
Labels
tech-debt Improves code quality or safety

Comments

@adlerjohn
Copy link
Contributor

if param.name == "gas_" || param.name == "amount_" || param.name == "color_" {

Currently, only fields with specific names are ignored. This isn't really documented anywhere and is the source of endless confusion. Moreover, currently the compiler requires exactly 4 fields from ABI methods: 3 implicit and one for user parameters. Therefore, there's no reason to prune on matching names when the first 3 can simply be pruned.

@adlerjohn adlerjohn added the tech-debt Improves code quality or safety label Jan 29, 2022
@iqdecay iqdecay self-assigned this Jan 31, 2022
@iqdecay
Copy link
Contributor

iqdecay commented Feb 3, 2022

So from my tests: currently, 14 exemples/tests don't give these 3 parameters, 9 do.
So there are 2 approaches:

  1. We require every example to provide them (annoying), I change the 14 examples
  2. I remove the parameters in the 9 remaining and I don't check for these parameters in the ABI

I'm thinking of going for (2). Otherwise, if we want freedom to either include them or not in the examples, we need to keep the name matching. But I think (2) is better, and we can "enforce" that people don't provide these arguments in their examples, and if they do, then it's just more annoying for them to call the encoded function.

@adlerjohn
Copy link
Contributor Author

Is it possible to instead have 2 abigen macros, one that ignores the first 3, makes sure there are exactly 4 parameters, and the types of the first 3 are as expected, and one that doesn't?

@iqdecay
Copy link
Contributor

iqdecay commented Feb 3, 2022

Yes I can do that but I cannot do it without resorting to name matching haha

@iqdecay
Copy link
Contributor

iqdecay commented Feb 3, 2022

image
Would that kind of approach suit you? And then I can propagate this strict_checking back to the macro def, which could even become a flag for the SDK

@adlerjohn
Copy link
Contributor Author

without resorting to name matching haha

Couldn't you just check the types of the first 3 args?

@iqdecay
Copy link
Contributor

iqdecay commented Feb 3, 2022

Ah yes sure, but it adds an "implicit" convention: if I feed it something that's not gas, color or amount first but has the right type, then it would not error whereas I would probably like to be warned that my arguments are discarded.

@adlerjohn
Copy link
Contributor Author

This is 1) a temporary thing until the compiler is improved and 2) users will always use ABI generated from the compiler. The only time where this implicit convention would be an issue is internally, but internally we just don't use the abigen flavor that removes the first 3 parameters (except to test that it works correctly of course).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech-debt Improves code quality or safety
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants