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

Adds concrete types distinction to JSON ABI. #599

Merged
merged 5 commits into from
Jul 24, 2024
Merged

Conversation

esdrubal
Copy link
Contributor

@esdrubal esdrubal commented Jul 16, 2024

FuelLabs/sway#5151
FuelLabs/sway#5952
FuelLabs/sway#5954

Rendered file

With the changes proposed in this PR, functions,loggedTypes, messagesTypes and configurables will only rely on hash based ids of concrete types.

This change is required because types on the types arrays can be abstract types, so the hash based ids could not include the generic parameters of the concrete types used.

For instance a method with two args, Option<u64> and Option<u32>, would generate two distinct hash based ids based on sha256("enum std::option::Option<u64>") and sha256("enum std::option::Option<u32>"), but there was a single type for Option.

With the proposed changes we can now have multiple hash based ids for generic types, while still having access to the same generated types as the new concreteTypes map easily to the typesMetadata (old types).

Before requesting review

  • I have reviewed the code myself

After merging, notify other teams

@esdrubal esdrubal added the ABI Contract ABI label Jul 16, 2024
@esdrubal esdrubal self-assigned this Jul 16, 2024
With the changes proposed in this PR, functions,loggedTypes,
messagesTypes and configurables will only rely on hash based ids of
concrete types.

This change is required because types on the types arrays can be
abstract types, so the hash based ids could not include the generic
parameters of the concrete types used.

For instance a method with two args, `Option<u64>` and `Option<u32>`,
would generate two distinct hash based ids based on `sha256("enum std::option::Option<u64>")`
and `sha256("enum std::option::Option<u32>")`, but there was a single type for Option.

With the proposed changes we can now have multiple hash based ids for
generic types, while still having access to the same generated types as
the new `concreteTypes` map easily to the `typesMetadata` (old `types`).
@esdrubal esdrubal requested review from a team July 16, 2024 14:19
src/abi/json-abi-format.md Outdated Show resolved Hide resolved
src/abi/json-abi-format.md Outdated Show resolved Hide resolved
src/abi/json-abi-format.md Outdated Show resolved Hide resolved
src/abi/json-abi-format.md Outdated Show resolved Hide resolved
esdrubal added a commit to FuelLabs/fuel-abi-types that referenced this pull request Jul 17, 2024
esdrubal added a commit to FuelLabs/fuel-abi-types that referenced this pull request Jul 17, 2024
src/abi/json-abi-format.md Outdated Show resolved Hide resolved
esdrubal added a commit to FuelLabs/fuel-abi-types that referenced this pull request Jul 18, 2024
esdrubal added a commit to FuelLabs/fuel-abi-types that referenced this pull request Jul 18, 2024
src/abi/json-abi-format.md Outdated Show resolved Hide resolved
src/abi/json-abi-format.md Outdated Show resolved Hide resolved
@arboleya arboleya requested a review from nedsalk July 23, 2024 10:51
@esdrubal esdrubal requested a review from arboleya July 23, 2024 16:29
Copy link

@hal3e hal3e left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the hard work!

@IGI-111 IGI-111 merged commit b095eb2 into master Jul 24, 2024
6 checks passed
@IGI-111 IGI-111 deleted the esdrubal/abi-changes2 branch July 24, 2024 14:17
IGI-111 pushed a commit to FuelLabs/fuel-abi-types that referenced this pull request Jul 24, 2024
FuelLabs/fuel-specs#599

---------

Co-authored-by: hal3e <git@hal3e.io>
],
"metadataTypes": [
{
"metadataTypeId": 1,
Copy link
Member

Choose a reason for hiding this comment

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

should we make metadataTypeId's string based in case we want to make them hashed later?

Br1ght0ne pushed a commit to FuelLabs/fuels-rs that referenced this pull request Jul 26, 2024
FuelLabs/fuel-specs#599

### Checklist

- [ ] I have linked to any relevant issues.
- [ ] I have updated the documentation.
- [ ] I have added tests that prove my fix is effective or that my
feature works.
- [ ] I have added necessary labels.
- [ ] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [ ] I have requested a review from the relevant team or maintainers.

---------

Co-authored-by: hal3e <git@hal3e.io>
- `"typeArguments"`: an array of the _type arguments_ used when applying the type of the message data being sent, if the type is generic, and `null` otherwise. Each _type argument_ is a _type application_ represented as a JSON object that contains the following properties:
- `"type"`: the _type declaration_ hash based ID of the type of the _type argument_.
- `"typeArguments"`: an array of the _type arguments_ used when applying the type of the _type argument_, if the type is generic, and `null` otherwise. The format of the elements of this array recursively follows the rules described in this section.
- `"message_id"`: a unique string ID.
Copy link
Member

Choose a reason for hiding this comment

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

My apologies for the post-merge feedback, but was there any particular reason why this is snake case? @esdrubal

IGI-111 added a commit to FuelLabs/sway that referenced this pull request Aug 15, 2024
## Description

Updates all the dependencies for the current release.

Implements the fuel ABI generation changes proposed in
FuelLabs/fuel-specs#599.

Removes the flag `--json-abi-with-callpaths` and the behavior is as if
it were true. We removed the flag because it is unsafe to produce JSON
ABIs without callpaths, so we shouldn't allow it.

Includes the LDC, BSIZ, BLDD and ED19 changes
from:#6409.

Fixes #5954
Fixes #5151

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [ ] If my change requires substantial documentation changes, I have
[requested support from the DevRel
team](https://github.com/FuelLabs/devrel-requests/issues/new/choose)
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [x] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.

---------

Co-authored-by: Vaivaswatha Nagaraj <vaivaswatha.nagaraj@fuel.sh>
Co-authored-by: Kaya Gokalp <kayagokalp123@gmail.com>
Co-authored-by: Kaya Gökalp <kaya.gokalp@fuel.sh>
Co-authored-by: Igor Rončević <ironcev@hotmail.com>
Co-authored-by: IGI-111 <igi-111@protonmail.com>
Co-authored-by: Sophie Dankel <47993817+sdankel@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ABI Contract ABI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants