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!: avoid add witnesses for predicates #2037

Merged
merged 103 commits into from
May 10, 2024

Conversation

Torres-ssf
Copy link
Contributor

Breaking Changes:

  • TransactionRequest.addPredicateResources was removed

@fuel-service-user
Copy link
Contributor

fuel-service-user commented Apr 11, 2024

✨ A PR has been created under the rc-2037 tag on fuels-wallet repo.
FuelLabs/fuels-wallet#1265

@Torres-ssf Torres-ssf marked this pull request as draft April 11, 2024 02:10
Torres-ssf and others added 2 commits May 6, 2024 08:59
Co-authored-by: Anderson Arboleya <anderson@arboleya.me>
Co-authored-by: Anderson Arboleya <anderson@arboleya.me>
@Torres-ssf Torres-ssf requested a review from arboleya May 6, 2024 11:59
arboleya
arboleya previously approved these changes May 7, 2024
Copy link
Contributor

@petertonysmith94 petertonysmith94 left a comment

Choose a reason for hiding this comment

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

Looks good to me 💚

packages/fuel-gauge/src/e2e-script.test.ts Outdated Show resolved Hide resolved
@Torres-ssf
Copy link
Contributor Author

Torres-ssf commented May 8, 2024

I am holding this until the fix #2262 is released.

I am unsure if the changes here will be reflected in more required updates on the PR that will upgrade https://github.com/FuelLabs/fuel-connectors to use the latest fuels version

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.

Nice one 🧙🏻

One thing that I like that they do in the RS SDK, is they will add funding / set gas limits to the exact amount required by the tx. So rather than an increase from 1000 > 2000, it would be increased to (ex. 1432). Then if we have a fail due to an increase consumption, we can actually validate that it should have increased. Thoughts?

@Torres-ssf
Copy link
Contributor Author

Nice one 🧙🏻

One thing that I like that they do in the RS SDK, is they will add funding / set gas limits to the exact amount required by the tx. So rather than an increase from 1000 > 2000, it would be increased to (ex. 1432). Then if we have a fail due to an increase in consumption, we can actually validate that it should have increased. Thoughts?

@danielbate I've failed to understand what you mean mate. Are you talking about adding tests to validate that a TX has consumed the amount of gas it was supposed to consume? And throw an error if they do consume a different value?

Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
79.66%(-0.14%) 69.79%(+0.16%) 77.6%(-0.23%) 79.76%(-0.15%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/account/src/predicate/predicate.ts 22.38%
(-7.03%)
31.25%
(-7.21%)
12.5%
(-4.16%)
21.42%
(-7.42%)
🔴 packages/account/src/providers/provider.ts 68.16%
(+0%)
55.97%
(+0.63%)
77.1%
(+0%)
68.39%
(+0%)
🔴 packages/account/src/providers/transaction-request/transaction-request.ts 85.51%
(-3.06%)
72.83%
(-1.85%)
86.53%
(-5.3%)
85.9%
(-2.98%)
🔴 packages/account/src/providers/transaction-request/utils.ts 87.5%
(+27.5%)
85.71%
(+45.71%)
100%
(+33.34%)
87.5%
(+27.5%)
🔴 packages/account/src/test-utils/seedTestWallet.ts 100%
(+0%)
60%
(+10%)
100%
(+0%)
100%
(+0%)

@Torres-ssf Torres-ssf merged commit ee969d3 into master May 10, 2024
19 checks passed
@Torres-ssf Torres-ssf deleted the st/fix/witness-entry-for-predicates branch May 10, 2024 18:25
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.

Transaction hash differs from the fuel-tx/vm
7 participants