FuelLabs / fuels-rs

Fuel Network Rust SDK
https://fuellabs.github.io/fuels-rs
Apache License 2.0
44.37k stars 1.33k forks source link

feat!: unfunded read only calls #1412

Open segfault-magnet opened 1 month ago

segfault-magnet commented 1 month ago

closes: #1405

Finishing a transaction builder (i.e. .build() now requires a Context object.

#[derive(Debug, Clone, Default)]
pub enum ScriptContext {
    /// Transaction is estimated and signatures are automatically added.
    #[default]
    Complete,
    /// Transaction is estimated but no signatures are added.
    /// Building without signatures will set the witness indexes of signed coins in the
    /// order as they appear in the inputs. Multiple coins with the same owner will have
    /// the same witness index. Make sure you sign the built transaction in the expected order.
    NoSignatures,
    /// No estimation is done and no signatures are added. Fake coins are added if no spendable inputs
    /// are present. Unused outputs are filled with variable outputs. Meant only for transactions that are to be dry-run with validations off.
    /// Useful for reading state with unfunded accounts.
    StateReadOnly,
}

The transaction builder will proceed to finalize the transaction in a manner fitting the context the tx will be used in.

Simulating a call now requires the user to tweak how the simulated call will be executed:

/// Used to control simulations/dry-runs
#[derive(Debug, Clone, Default)]
pub enum Execution {
    /// The transaction will be subject to all validations -- the tx fee must be covered, witnesses
    /// and UTXOs must be valid, etc.
    #[default]
    Realistic,
    /// Most validation is disabled. Witnesses are replaced with fake ones, fake base assets are
    /// added if necessary. Unused outputs are filled with variable outputs. Useful for fetching state without needing an account
    /// with base assets.
    StateReadOnly,
}

And that allows for actual read only calls

    let no_funds_wallet = WalletUnlocked::new_random(provider);

    let response = MyContract::new(contract_id, no_funds_wallet.clone())
        .methods()
        .get(5, 6)
        .simulate(Execution::StateReadOnly)
        .await?;

Breaking changes

MujkicA commented 2 weeks ago

I have some reservations about this that I've been discussing with @hal3e.

I don't fully understand the use case for NoSignatures. We are essentially putting another abstraction over how witness indexes are handled but from my perspective it seems leaky in this case. It might absolve the user from handling the witness indexes directly but it still requires them to be somewhat aware of what is happening behind the scenes. Since we are in the process of re-examining our interfaces for v1.0, I would suggest we hold off on this change for now until we have better picture of how we want to support transaction building and what we want the final interface to look like. At the very least, I would hold off until @segfault-magnet can chime in on the discussion.

segfault-magnet commented 2 weeks ago

I have some reservations about this that I've been discussing with @hal3e.

I don't fully understand the use case for NoSignatures. We are essentially putting another abstraction over how witness indexes are handled but from my perspective it seems leaky in this case. It might absolve the user from handling the witness indexes directly but it still requires them to be somewhat aware of what is happening behind the scenes. Since we are in the process of re-examining our interfaces for v1.0, I would suggest we hold off on this change for now until we have better picture of how we want to support transaction building and what we want the final interface to look like. At the very least, I would hold off until @segfault-magnet can chime in on the discussion.

NoSignatures is already there in master, called build_without_signatures. Whatever its reason for existence, the leakyness is there. This is a cosmetic change wrt.

If we know we will allocate time for 1.0 redesign and if this feature (and all others that break the public api) can wait for the redesign then I vote for waiting.

Then again 1.0 will break everything so why not break this as well? We could get some feedback in the meantime if there are edge cases to the approach.

digorithm commented 2 weeks ago

I have some reservations about this that I've been discussing with @hal3e. I don't fully understand the use case for NoSignatures. We are essentially putting another abstraction over how witness indexes are handled but from my perspective it seems leaky in this case. It might absolve the user from handling the witness indexes directly but it still requires them to be somewhat aware of what is happening behind the scenes. Since we are in the process of re-examining our interfaces for v1.0, I would suggest we hold off on this change for now until we have better picture of how we want to support transaction building and what we want the final interface to look like. At the very least, I would hold off until @segfault-magnet can chime in on the discussion.

NoSignatures is already there in master, called build_without_signatures. Whatever its reason for existence, the leakyness is there. This is a cosmetic change wrt.

If we know we will allocate time for 1.0 redesign and if this feature (and all others that break the public api) can wait for the redesign then I vote for waiting.

Then again 1.0 will break everything so why not break this as well? We could get some feedback in the meantime if there are edge cases to the approach.

Yeah. Let's release this feature now; our users visibly need it (and often request it from me). We can always keep making breaking changes until 1.0. I'll continue handling the communications for these painful, breaking changes.

digorithm commented 1 week ago

@hal3e are we ready to merge this?