-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: Add attributes
to ABIFunction
#744
Conversation
This is needed for FuelLabs/sway#3450.
5510d2f
to
9f2ac17
Compare
This seems to be a breaking change. The CI is complaining:
I haven't checked more closely but it seems that the way we're adding this is breaking backward compatibility -- i.e. we won't be able to release until the compiler starts providing the attribute field. Do you have an ETA this? If it's too long we should probably look into guiding serde to default-populate the field if it is not present in the json file and still remain backward compatible. Also, unless deserialization is altered, you'll probably need a feature flag in forc so that it may generate the appropriate json files with the attributes present, otherwise the CI can't generate the abi json files needed for our e2e tests. |
@segfault-magnet I made the function attributes optional -- it turns out to be too hard to introduce a breaking change like an obligatory field because of close coupling of the Sway and Fuels-rs repos. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This is needed for FuelLabs/sway#3450 and for #742 eventually. Spec issue: FuelLabs/fuel-specs#446 Compiler change: FuelLabs/sway#3450
This is needed for FuelLabs/sway#3450 and for #742 eventually.
Spec issue: FuelLabs/fuel-specs#446
Compiler change: FuelLabs/sway#3450
Correction: the explanation below is only true at the parser level, the distinction between
foo
andfoo()
disappears later in the pipeline, so I changed the type ofarguments
field toVec<String>
.The
Option<Vec<String>>
inis needed because in Sway a parameterless attribute
foo
and an attribute with empty parameter listfoo()
are not the same.