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

fix: handle encoding and fn_selector resolving of raw untyped ptr #565

Merged
merged 7 commits into from
Oct 31, 2022

Conversation

segfault-magnet
Copy link
Contributor

closes: #547

Not sure if there is anything else that needs bumping when doing a forc update.

@segfault-magnet segfault-magnet added the bug Issue is a bug label Oct 31, 2022
@segfault-magnet segfault-magnet requested review from LuizAsFight and a team October 31, 2022 08:51
@segfault-magnet segfault-magnet self-assigned this Oct 31, 2022
@segfault-magnet
Copy link
Contributor Author

Also not sure what to do about the CI lint phase failing:

🦋  error Some packages have been changed but no changesets were found. Run `changeset add` to resolve this error.
🦋  error If this change doesn't need a release, run `changeset add --empty`.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 2022

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
90.14% (-0.15% 🔻)
3558/3947
🟡 Branches
72.58% (-0.47% 🔻)
696/959
🟢 Functions
87.99% (-0.37% 🔻)
718/816
🟢 Lines
90.16% (-0.15% 🔻)
3408/3780
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟡
... / transaction-response.ts
79.41% (-5.88% 🔻)
44.44% 100%
79.41% (-5.88% 🔻)
🟢
... / scripts.ts
94.23% (-1.92% 🔻)
81.25% (-6.25% 🔻)
100%
94% (-2% 🔻)
🟢
... / errors.ts
85% (-10% 🔻)
43.75% (-18.75% 🔻)
62.5% (-25% 🔻)
85% (-10% 🔻)
🟡 script/src/utils.ts
66.67% (-33.33% 🔻)
0% (-60% 🔻)
50% (-50% 🔻)
62.5% (-37.5% 🔻)

Test suite run success

541 tests passing in 49 suites.

Report generated by 🧪jest coverage report action from 42f0bbc

@@ -1,7 +1,7 @@
{
"private": true,
"name": "forc-bin",
"version": "0.26.1",
Copy link
Member

Choose a reason for hiding this comment

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

This routine was recently updated. You don't need to update the version field anymore, only config.forcVersion — which you did already.

The version is now managed automatically via the generated changesets as part of the general release routine.

@arboleya
Copy link
Member

Not sure if there is anything else that needs bumping when doing a forc update.

Is this a breaking change? If so, I believe a minor bump. Otherwise, a patch one.

@luizstacio Please confirm. 👆

Also not sure what to do about the CI lint phase failing:

Please, implement the changes I request here.

Then re-generate the changeset — pnpm changeset:next.

@segfault-magnet
Copy link
Contributor Author

Is this a breaking change?

After this is merged, fuels-ts vector support will cease for forc < v0.28.0 and function only for v0.28.0+.

Dunno if that broke enough things for a minor bump :)

@segfault-magnet
Copy link
Contributor Author

Then re-generate the changeset — pnpm changeset:next.

I haven't used changeset. Can you clear up a few points for me? @arboleya
I ran pnpm changeset:next and it created a file fuels-ts/.changeset/fuel-labs-ci.md with the following content:

---
"fuels": patch
---

incremental

I'm guessing the command didn't have any other side effects besides this. Am I to commit this file and when a release is done the CI will know to bump the version of fuels? Will this bump the forc-bin version I previously had changed manually? Or should I have run the command in the forc-bin package itself?

Also, in the file, it says patch. Still waiting for a verdict on that one, but if it ends up being 'do a minor bump', am I to change patch -> minor and all is well?

Hopefully, the CI gets its forc binary from the forcVersion and not the package version of fuel-bin otherwise it is going to fail due to the forc version being 0.26.something

@luizstacio
Copy link
Member

Then re-generate the changeset — pnpm changeset:next.

I haven't used changeset. Can you clear up a few points for me? @arboleya I ran pnpm changeset:next and it created a file fuels-ts/.changeset/fuel-labs-ci.md with the following content:

---
"fuels": patch
---

incremental

I'm guessing the command didn't have any other side effects besides this. Am I to commit this file and when a release is done the CI will know to bump the version of fuels? Will this bump the forc-bin version I previously had changed manually? Or should I have run the command in the forc-bin package itself?

Also, in the file, it says patch. Still waiting for a verdict on that one, but if it ends up being 'do a minor bump', am I to change patch -> minor and all is well?

Hopefully, the CI gets its forc binary from the forcVersion and not the package version of fuel-bin otherwise it is going to fail due to the forc version being 0.26.something

To add a changeset you should run the command pnpm changeset add this will show some instructions and ask you to add the summary, the summary should be an understandable text for the change, in order to improve the changelog.

@segfault-magnet
Copy link
Contributor Author

Then re-generate the changeset — pnpm changeset:next.

I haven't used changeset. Can you clear up a few points for me? @arboleya I ran pnpm changeset:next and it created a file fuels-ts/.changeset/fuel-labs-ci.md with the following content:

---
"fuels": patch
---

incremental

I'm guessing the command didn't have any other side effects besides this. Am I to commit this file and when a release is done the CI will know to bump the version of fuels? Will this bump the forc-bin version I previously had changed manually? Or should I have run the command in the forc-bin package itself?
Also, in the file, it says patch. Still waiting for a verdict on that one, but if it ends up being 'do a minor bump', am I to change patch -> minor and all is well?
Hopefully, the CI gets its forc binary from the forcVersion and not the package version of fuel-bin otherwise it is going to fail due to the forc version being 0.26.something

To add a changeset you should run the command pnpm changeset add this will show some instructions and ask you to add the summary, the summary should be an understandable text for the change, in order to improve the changelog.

Great, ty. The UX is friendly enough.

The only thing left is to figure out the severity of the version bumps for packages abi-coder and forc-bin.

Any thoughts?

@arboleya
Copy link
Member

@segfault-magnet I've forgotten we had this, apologies. As Luiz pointed out, the pnpm changeset add seems to be the correct command. And yes, the CI should read the forcVesion config.

Let's see what @luizstacio says about the minor x patch dilemma.

@luizstacio
Copy link
Member

@segfault-magnet I've forgotten we had this, apologies. As Luiz pointed out, the pnpm changeset add seems to be the correct command. And yes, the CI should read the forcVesion config.

Let's see what @luizstacio says about the minor x patch dilemma.

Both work, no problem to use a patch, the only restricted thing, for now, is to use major.
On wich to choose @arboleya was the correct minor for breaks and patches for small changes.

@segfault-magnet
Copy link
Contributor Author

Added the changeset. Did patch bumps on both packages.

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.

LGTM.

Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

:shipit:

Thanks @segfault-magnet for taking the time to work through this ❤️

@segfault-magnet segfault-magnet merged commit bdfa9d6 into master Oct 31, 2022
@segfault-magnet segfault-magnet deleted the segfault-magnet/raw_untyped_ptr branch October 31, 2022 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support raw_ptr in the JSON ABI encoder/decoder
4 participants