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!: refactored the getTransactionCost method #2643

Merged
merged 40 commits into from
Jul 23, 2024

Conversation

petertonysmith94
Copy link
Contributor

@petertonysmith94 petertonysmith94 commented Jun 28, 2024

Release notes

In this release, we:

  • Refactored getTransactionCost method for Provider and Account

Summary

  • Refactored functionality for Provider.getTransactionCost to Account.getTransactionCost:
    • Separate concerns of estimating and fake funding a transaction.
    • Reducing unnecessary parameters, leading to a cleaner API.
  • Deprecated the Provider.getResourcesForTransaction method:
    • Lack of use-cases and deemed not required.

Breaking Changes

Refactored functionality for Provider.getTransactionCost to Account.getTransactionCost and changed estimation parameter from quantitiesToContract to quantities.

// before
const provider = Provider.create(...);
const account = Wallet.generate({ ... }) || new Predicate(...);
const quantities: Array<CoinQuantityLike> = [
  { amount: 1000, assetId: provider.getBaseAssetId() }
];

const cost = provider.getTransactionCost(txRequest, {
  resourceOwner: account,
  quantitiesToContract: quantities,
})
// after
const provider = Provider.create(...);
const account = Wallet.generate({ ... }) || new Predicate(...);
const quantities: Array<CoinQuantityLike> = [
  { amount: 1000, assetId: provider.getBaseAssetId() }
];

const cost = account.getTransactionCost(txRequest, { quantities });

Checklist

  • I addedtests to prove my changes
  • I updated — all the necessary docs
  • I reviewed — the entire PR myself, using the GitHub UI
  • I described — all breaking changes and the Migration Guide

@petertonysmith94 petertonysmith94 added the chore Issue is a chore label Jun 28, 2024
@petertonysmith94 petertonysmith94 added this to the 0.x mainnet milestone Jun 28, 2024
@petertonysmith94 petertonysmith94 self-assigned this Jun 28, 2024
Copy link
Contributor

@Torres-ssf Torres-ssf left a comment

Choose a reason for hiding this comment

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

Hey @petertonysmith94 I understand that this is a draft yet, just leave a couple of questions here to better understand your motivations

Copy link
Contributor Author

@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.

@Torres-ssf really appreciate the review on the draft :)

packages/account/src/account.ts Outdated Show resolved Hide resolved
packages/account/src/account.ts Outdated Show resolved Hide resolved
packages/program/src/functions/base-invocation-scope.ts Outdated Show resolved Hide resolved
@petertonysmith94 petertonysmith94 changed the title chore: refactored the getTransactionCost method chore!: refactored the getTransactionCost method Jul 8, 2024
@arboleya arboleya removed this from the 0.x mainnet milestone Jul 19, 2024
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.

Conflicts to fix.

Copy link
Contributor

@Torres-ssf Torres-ssf left a comment

Choose a reason for hiding this comment

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

@petertonysmith94 I feel like we need to update this doc page to say that the getTransactionCost method is provided by the Account class instead of the Provider class, what do you think?

packages/account/src/account.ts Outdated Show resolved Hide resolved
Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
79.4%(+0.04%) 71.53%(-0.12%) 77.32%(+0.02%) 79.53%(+0.04%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/account/src/account.ts 82.31%
(+1.16%)
65.07%
(+1.14%)
82.35%
(+0.54%)
82%
(+1.15%)
🔴 packages/account/src/providers/provider.ts 68.02%
(-0.82%)
62.63%
(-1.19%)
73.49%
(-0.31%)
67.98%
(-0.81%)
🔴 packages/account/src/test-utils/launchNode.ts 98.27%
(+0.01%)
85.24%
(+0.5%)
100%
(+0%)
98.36%
(+0.02%)
🔴 packages/contract/src/contract-factory.ts 58.33%
(-0.29%)
46.42%
(+0.27%)
54.54%
(+0%)
58.33%
(-0.29%)
🔴 packages/contract/src/util.ts 93.75%
(+0.42%)
0%
(+0%)
66.66%
(+0%)
88.88%
(+0.65%)
🔴 packages/create-fuels/src/cli.ts 85.5%
(+0.21%)
37.5%
(-2.5%)
100%
(+0%)
85.5%
(+0.21%)

Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

Nice job!

@petertonysmith94 petertonysmith94 merged commit d4c4e55 into master Jul 23, 2024
25 checks passed
@petertonysmith94 petertonysmith94 deleted the ps/chore/refactor-get-transaction-cost branch July 23, 2024 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Issue is a chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal to Simplify Provider.getTransactionCost
4 participants