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!: beta-5 support #1200

Merged
merged 66 commits into from
Nov 17, 2023
Merged

feat!: beta-5 support #1200

merged 66 commits into from
Nov 17, 2023

Conversation

hal3e
Copy link
Contributor

@hal3e hal3e commented Nov 11, 2023

This PR adds sdk support for beta-5.

  • TODO: update docs and describe breaking changes

BREAKING CHANGE:

  • TxParameters are replaced with TxPolicies
  • GasPrice and Maturity fields are optional
  • TxPolicies introduced new fields:
    • WitnessLimit - allows the limitation of the maximum size of witnesses in bytes.
    • MaxFee - allows the upper bound for the maximum fee that users agree to pay for the transaction.
  • The ScriptGasLimit only limits the script execution. Previously, the ScriptGasLimit also limited the predicate execution time, but it is not valid anymore. So, it is not possible to use the ScriptGasLimit for transaction cost limitations. A new MaxFee policy is a way to do that. The GasLimit field was removed from the Create transaction because it only relates to the script execution(which the Create transaction doesn't have).
  • The new WhitnessLimit also impacts the max_gas and max_fee calculation along with the ScriptGasLimit(in the case of Create transaction only WitnessLimit affects the max_gas and max_fee).
  • The minimal gas also charges the user for transaction ID calculation.
  • Each transaction requires setting the GasPrice policy.
  • Previously, GasLimit should be less than the MAX_GAS_PER_TX constant. After removing this field from the Create transaction, it is impossible to require it. Instead, it requires that max_gas <= MAX_GAS_PER_TX for any transaction. Consequently, any Script transaction that uses MAX_GAS_PER_TX as a GasLimit will always fail because of a new rule. Setting the estimated gas usage instead solves the problem.
  • If the max_fee > policies.max_fee, then transaction will be rejected.
  • If the witnessses_size > policies.witness_limit, then transaction will be rejected.
  • get_message_proof not uses Nonce instead of the message_id
  • predicates do not use ChainId for address calculation
  • manual_blocks_enabled is replaced with debug in local chain config
  • fee_checked_from_tx uses FeeParameters
  • fuel_tx::ConsensusParameters were refactored - this also affects us
  • when building a transaction_builder the BuildableTransacion trait needs to be in scope
  • utxo_validation and manual_blocks are enabled by default for test providers
  • node config does not have local_node anymore use default - let node_config = Config::default();

Thanks @MujkicA for updating the documentation.

Checklist

  • I have linked to any relevant issues.
  • I have updated the documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary labels.
  • 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.

@hal3e hal3e added big breaking Introduces or requires breaking changes labels Nov 11, 2023
@hal3e hal3e self-assigned this Nov 11, 2023
.github/workflows/ci.yml Outdated Show resolved Hide resolved
docs/src/calling-contracts/tx-params.md Outdated Show resolved Hide resolved
examples/contracts/src/lib.rs Show resolved Hide resolved
packages/fuels-accounts/src/account.rs Outdated Show resolved Hide resolved
packages/fuels-accounts/src/account.rs Outdated Show resolved Hide resolved
packages/fuels-core/src/types/wrappers/transaction.rs Outdated Show resolved Hide resolved
packages/fuels-core/src/types/transaction_builders.rs Outdated Show resolved Hide resolved
packages/fuels-core/src/types/transaction_builders.rs Outdated Show resolved Hide resolved
packages/fuels/tests/providers.rs Outdated Show resolved Hide resolved
MujkicA and others added 4 commits November 16, 2023 14:56
Co-authored-by: Ahmed Sagdati <37515857+segfault-magnet@users.noreply.github.com>
@digorithm
Copy link
Member

Adding to @iqdecay's

  • I'm not sure I understand the logic behind renaming everything. To me, we are exposing implementation details to the user. As a user, using TxPolicies instead of TxParameters feels unusual at best. Also I feel like our responsability as the SDK is to shield the user from all the little internal changes the fuel-core or fuel-vm repos go through. So I'm not sure why we are replicating all these breaking changes in our API

Think of "transaction policies" as the new domain language here. This is coming from upstream (fuel-core), meaning that if we somehow wrap it around our own `TxParameters" language, we will be causing a lot of confusion in the future when this abstraction inevitably leaks. Sticking with one language is best for communicating. A lot of users will be talking about the configurations for transaction policies. If the SDK forces it to be called transaction parameters, it'll just be confusing!

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.

Thanks @MujkicA for the doc updates.

Code comments are also much better now.

Just a few nits left.

docs/src/calling-contracts/tx-policies.md Outdated Show resolved Hide resolved
docs/src/calling-contracts/tx-policies.md Outdated Show resolved Hide resolved
iqdecay
iqdecay previously approved these changes Nov 17, 2023
Copy link
Contributor

@iqdecay iqdecay left a comment

Choose a reason for hiding this comment

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

looking good! Thanks for your answers, it's clearer to me now. Left one formatting nit.

docs/src/calling-contracts/tx-policies.md Outdated Show resolved Hide resolved
docs/src/calling-contracts/tx-policies.md Outdated Show resolved Hide resolved
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.

LFG! 🚀

@hal3e hal3e merged commit b9cac99 into master Nov 17, 2023
39 checks passed
@hal3e hal3e deleted the hal3e/transaction-policies branch November 17, 2023 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
big breaking Introduces or requires breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants