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

feat: add function for generating bytecode identifier #6674

Merged
merged 69 commits into from
Nov 19, 2024

Conversation

sdankel
Copy link
Member

@sdankel sdankel commented Oct 25, 2024

Description

Depends on #6522
Related FuelLabs/forc.pub#16

Adds a function, get_bytecode_id that generates a sha256 hash of the bytecode with the configurables section of the bytecode removed. This will be used for indexing and lookups of the corresponding ABIs for contracts, predicates, and scripts in the package registry (forc.pub).

Checklist

  • I have linked to any relevant issues.
  • 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).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

vaivaswatha
vaivaswatha previously approved these changes Oct 31, 2024
@sdankel sdankel dismissed stale reviews from vaivaswatha and alfiedotwtf via 87257e9 November 3, 2024 00:13
@sdankel sdankel marked this pull request as draft November 3, 2024 00:39
@sdankel
Copy link
Member Author

sdankel commented Nov 4, 2024

This is currently blocked because the related compiler change was reverted.

@sdankel sdankel marked this pull request as ready for review November 18, 2024 15:10
@sdankel sdankel requested a review from a team November 19, 2024 11:05
Copy link
Member

@JoshuaBatty JoshuaBatty left a comment

Choose a reason for hiding this comment

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

Nice! Appreciate all the comments done here as well.

@zees-dev
Copy link
Contributor

Great work!

Do you think it would be worthwhile having the source .sw files for the contracts aswell as the binary files?
I can potentially imagine a scenario where we could have compiler changes, and hence the output binaries may not be regeneratable.
Having the source alongside would potentially help resolve such future test failures i'd imagine.

@sdankel
Copy link
Member Author

sdankel commented Nov 19, 2024

Great work!

Do you think it would be worthwhile having the source .sw files for the contracts aswell as the binary files? I can potentially imagine a scenario where we could have compiler changes, and hence the output binaries may not be regeneratable. Having the source alongside would potentially help resolve such future test failures i'd imagine.

Thanks for bringing this up. I thought about having the tests generate the binary files from the source, but thought that it would be annoying to have to update the bytecode IDs in the tests whenever the compiler changes.

I figured that if the tests aren't regenerating the binary from source, there was no need to include the source, since the source projects are already checked in under examples/

When we need to regenerate the binary files in the future, it would just be a matter of building those two examples with forc build and forc build --release and updating the bytecode IDs in the tests. Let me know what you think.

@zees-dev
Copy link
Contributor

I figured that if the tests aren't regenerating the binary from source, there was no need to include the source, since the source projects are already checked in under examples/

Didn't know the the source is already in examples; in that case no need for another source.
Agree that compilation would still be issue regardless - if compiler changes.

Just thinking from the point of someone else having to fix the tests if they break.
A comment in the test(s) themselves could help (point to examples).
Nevertheless will leave this upto you.

@sdankel
Copy link
Member Author

sdankel commented Nov 19, 2024

Just thinking from the point of someone else having to fix the tests if they break. A comment in the test(s) themselves could help (point to examples). Nevertheless will leave this upto you.

Good idea. I'll add a comment in a separate PR since this one is pretty much ready to go.

@sdankel sdankel merged commit 6245ee6 into master Nov 19, 2024
39 checks passed
@sdankel sdankel deleted the sophie/bytecode-id branch November 19, 2024 16:49
JoshuaBatty added a commit that referenced this pull request Nov 20, 2024
## Description

Adding comments per
#6674 (comment)

## Checklist

- [ ] I have linked to any relevant issues.
- [ ] 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)
- [ ] I have added tests that prove my fix is effective or that my
feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [ ] 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: JoshuaBatty <joshpbatty@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants