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

chore: remove sway-libs dep from testing fixtures #2189

Merged
merged 20 commits into from May 14, 2024

Conversation

maschad
Copy link
Member

@maschad maschad commented Apr 26, 2024

Closes #2160

  • Mocks the compute_bytecode functions in the sway contracts

@maschad maschad self-assigned this Apr 26, 2024
@maschad maschad added the chore label Apr 26, 2024
@maschad maschad added blocked and removed blocked labels May 1, 2024
@maschad maschad marked this pull request as ready for review May 7, 2024 21:20
@maschad maschad requested a review from arboleya May 7, 2024 21:33
@maschad
Copy link
Member Author

maschad commented May 7, 2024

@Dhaiwat10 @arboleya any ideas why this test would hang on CI? It works fine locally
Screenshot 2024-05-07 at 4 33 09 PM

Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

any ideas why this test would hang on CI? It works fine locally

Nothing I can see only by reading it.

apps/docs-snippets/src/guide/types/vector.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@danielbate danielbate left a comment

Choose a reason for hiding this comment

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

One concern here is the docs still references the sway std lib function, so we are miss communicating the dependency. If the sway std lib signature changed, we are not in sync as we have a mocked version. I think we should be more generally referencing this as working with bytes with both a Vec<u8> and Bytes example. Thoughts?

@maschad
Copy link
Member Author

maschad commented May 10, 2024

One concern here is the docs still references the sway std lib function, so we are miss communicating the dependency. If the sway std lib signature changed, we are not in sync as we have a mocked version. I think we should be more generally referencing this as working with bytes with both a Vec<u8> and Bytes example. Thoughts?

Whether it's mocked or not wouldn't have an effect as it relates to breaking changes as we would still be using a particular version as our dependency

the benefit with this at least would be should we try to upgrade, if there is a breaking change, Rust would let us know due to it's semver toolchain - but this would still be dependent on our manual attempt to upgrade.

In summary, I'm not sure if there is an uncomplicated way to discuss working with bytecode or Vec<u8> without using a concrete example from a sway library.

@danielbate
Copy link
Contributor

Whether it's mocked or not wouldn't have an effect as it relates to breaking changes as we would still be using a particular version as our dependency

But agreed we should remove the dependency, like you have done.

In summary, I'm not sure if there is an uncomplicated way to discuss working with bytecode or Vec without using a concrete example from a sway library.

I would personally prefer removing the reference to the sway std lib in the docs and just more generally speak about bytecode.

Some functions require you to pass in bytecode to the function. The type of the bytecode parameter is usually `Vec<u8>`, this would be handled like so:

<-- code snippet -->

@maschad
Copy link
Member Author

maschad commented May 13, 2024

I would personally prefer removing the reference to the sway std lib in the docs and just more generally speak about bytecode.

2f6af7d

Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
79.54%(+0%) 69.84%(+0%) 77.12%(+0%) 79.65%(+0%)
Changed Files:

Coverage values did not change👌.

@maschad maschad merged commit 054c1dc into master May 14, 2024
19 checks passed
@maschad maschad deleted the fix/remove-sway-libs-dep branch May 14, 2024 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore: remove sway-libs dependency
7 participants