Set custom transaction inputs and outputs for predicates

[supiket]

Implement with_inputs() and with_outputs() functionality for predicates (#726).

@hal3e

[hal3e]

Team, I need your feedback on this.

In the OTC-swap-predicate they used the ScriptBuilder to create a script with custom inputs and outputs. Here is the code:

let script = ScriptBuilder::new()
    .set_inputs(vec![input_predicate, input_from_taker])
    .set_outputs(vec![
        output_to_receiver,
        output_to_taker,
        output_asked_change,
    ])
    .set_gas_limit(10_000_000)
    .set_script(vec![Opcode::RET(REG_ONE)]);

Note that the input_predicate is as Input::CoinPredicate

In the new releases of the SDK we can not use this code directly as we do not have the ScriptBuilder anymore.

So now we have 3 options:

1. Let the user have the freedom to create inputs as he wants, i.e. create CoinPredicates. And use the wallet to make a transfer like so:

let tx_parameters = TxParameters {
    gas_limit: 10_000_000,
    ..Default::default()
};

let mut tx = Wallet::build_transfer_tx(
    &[input_predicate, input_from_taker],
    &[output_to_receiver, output_to_taker, output_asked_change],
    tx_parameters,
);

taker_wallet.sign_transaction(&mut tx).await.unwrap();
let _receipts = provider.send_transaction(&tx).await.unwrap();

2. Demand Input::Coin / Messages from the user and change those to Coin / Message Predicates for them. We should do this because if we use dynamic data as input we need to calculate the right offset. In the first option, the user should calculate the offset themself. However, it is also not intuitive that we will change the coins to predicate coins for them. What makes the thing more complicated is that here they have also a CoinSigned input which should not be changed :/

If we go with this option. I would add a new methods to the predicate_abigen to add and change the inputs, outputs and a new spend method that would use them.

3. Let the user make a custom transaction as in option 1, but provide a separate function that converts Coins/Messages to Coin/Message Predicate with the right offsets. This would not affect the predicate_abigen directly but it could be confusing, i.e. why we sometimes use the predicate abigen and sometimes not.

Note that one big selling point of the predicate abigen was the ability to nicely encode the input data. I lean towards option 2. Otherwise the user would have to encode the data himself or use the abigen to do it and still make custom inputs which could again be confusing :/

What do you think.

@FuelLabs/sdk-rust @supiket

[supiket]

Option 1 or potentially 3 seems the most suitable to me, but if having a function to convert Coin/Message to Coin/Message Predicate has no advantage other than setting offsets correctly, then adding another layer of abstraction is not worth it in my opinion.

Interacting with a predicate requires knowing exactly what the predicate does and the transaction inputs and outputs. The way to interact with them would vary depending on what the predicate does, so I don't think there is a way to work around setting inputs and outputs in a custom way unless there exists a general pattern that almost always holds, which as far as I know, doesn't exist.

I'm for more flexibility and the less abstraction, what do you think @simonr0204?

[hal3e]

Just to add some context to the conversation...

IMO setting the right offset and encoding the data is not trivial. That is why I am so conflicted. In this example, you did not have to deal with it directly but if you had to you would have had to manually do something like this:

// Iterate through the spendable resources and calculate the appropriate offsets
// for the coin or message predicates
let mut offset = base_predicate_offset(&predicate.consensus_parameters().await?);
let inputs = spendable_predicate_resources
    .into_iter()
    .map(|resource| match resource {
        Resource::Coin(coin) => {
            offset += coin_predicate_data_offset(code.len());

            let data = predicate_data.clone().resolve(offset as u64);
            offset += data.len();

            self.create_coin_predicate(coin, asset_id, code.clone(), data)
        }
        Resource::Message(message) => {
            offset += message_predicate_data_offset(message.data.len(), code.len());

            let data = predicate_data.clone().resolve(offset as u64);
            offset += data.len();

            self.create_message_predicate(message, code.clone(), data)
        }
    })
    .collect::<Vec<_>>();

The code above uses Resource but it would be the same for Inputs. Here we start with a fixed offset and then for each Coin / Message add an appropriate offset and resolve the data.

[digorithm]

This is a tough UX problem, so I'm still in the thinking process. My thoughts so far...

Option 1 or potentially 3 seems the most suitable to me, but if having a function to convert Coin/Message to Coin/Message Predicate has no advantage other than setting offsets correctly, then adding another layer of abstraction is not worth it in my opinion.

Interacting with a predicate requires knowing exactly what the predicate does and the transaction inputs and outputs. The way to interact with them would vary depending on what the predicate does, so I don't think there is a way to work around setting inputs and outputs in a custom way unless there exists a general pattern that almost always holds, which as far as I know, doesn't exist.

I'm for more flexibility and the less abstraction, what do you think @simonr0204?

This block right here by @supiket is extremely on point. All three paragraphs are on point; the summary is that adding unnecessary abstraction layers is a bad idea. If you're working with predicates, you're operating at the Input/'Output abstraction level; no need to abstract that away.

That said, regardless if you're working with Predicate, Script, or Contract, you're definitely not working at the Offset abstraction level; i.e., the user should never know about offsets; it's way too low-level.

Now, the question is, does approach (1) really require the user to compute the offsets themselves? If yes, we must introduce an abstraction layer because we don't want users to think about Offsets, and if that's a goal, then this abstraction layer isn't unnecessary.

[hal3e]

Now, the question is, does approach (1) really require the user to compute the offsets themselves? If yes, we must introduce an abstraction layer because we don't want users to think about Offsets, and if that's a goal, then this abstraction layer isn't unnecessary.

The answer is unfortunately yes and no. If they are not using dynamic data like vectors then they would not have to deal with offsets. As soon as they have dynamic data as input, they need the correct offsets. Debugging in this case is very hard because you only get a revert error which states that the predicate failed to validate.

[supiket]

After our latest talk on this, have we decided to go with the option that abstracts the offsets from users, i.e. option 3?

[hal3e]

Yes that is correct. I will make a PR with the initial implementation and we can continue the discussion there.

[hal3e]

@Salka1988 and @MujkicA is this issue obsolete now that #815 is merged ?

[Salka1988]

@Salka1988 and @MujkicA is this issue obsolete now that #815 is merged ?

Yes, you're right. ScriptTransactionBuilder and CreateTransactionBuilder can be used if we want to build transaction with custom inputs and outputs.

For exemple:

        ScriptTransactionBuilder::default().set_inputs(inputs).set_outputs(outputs).build();
        CreateTransactionBuilder::default().set_inputs(inputs).set_outputs(outputs).build();

This works for both wallets and predicates. For more details see packages/fuels-types/src/transaction_builders.rs.