-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat!: add support for assemble tx #1634
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits and style opinions but otherwise LGTM.
#[allow(clippy::too_many_arguments)] | ||
pub async fn assemble_tx( | ||
&self, | ||
transaction: impl Transaction, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think we can reduce this requirement since we only use .into()
. Unless you believe we'll call more of the Transaction
trait methods in the future.
transaction: impl Transaction, | |
transaction: impl Into<FuelTransaction>, |
async fn update_witnesses<T: UniqueIdentifier + Witnesses + Inputs>( | ||
tx: &mut T, | ||
unresolved_signers: &[Arc<dyn Signer + Send + Sync>], | ||
chain_id: &ChainId, | ||
) -> Result<()> { | ||
let id = tx.id(chain_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're already using generics here, it's unnecessarily restrictive to enforce the use of trait objects for unresolved_signers
. I'd instead prefer something like:
async fn update_witnesses<T: UniqueIdentifier + Witnesses + Inputs>( | |
tx: &mut T, | |
unresolved_signers: &[Arc<dyn Signer + Send + Sync>], | |
chain_id: &ChainId, | |
) -> Result<()> { | |
let id = tx.id(chain_id); | |
async fn update_witnesses<T: UniqueIdentifier + Witnesses + Inputs, S: Signer>( | |
tx: &mut T, | |
unresolved_signers: &[S], | |
chain_id: &ChainId, | |
) -> Result<()> { | |
let id = tx.id(chain_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signers might be of different types, ie you cannot have a slice of &[AwsSigner, PrivateKeySigner, LedgerSigner].
S
is only going to resolve to one type at at time making the unresolved signers a slice of that particular signer type.
The builder has a add_signer(&mut self,...
that accepts a dyn Signer
so that is where the unresolved_signers
come from.
TLDR,
we chose type erasure for the tx builder so that we don't end up with ScriptTxBuilder<NextSigner<PrivateKeySigner, NextSigner<AwsSigner, LedgerSigner>>>
or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It simplify so much things that's soooo cool. Nice one !
@@ -170,6 +173,21 @@ pub trait Account: ViewOnlyAccount { | |||
Ok(()) | |||
} | |||
|
|||
fn required_balance( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add document such as the other methods of this trait
let mut policies = Policies::default(); | ||
|
||
policies.set(PolicyType::WitnessLimit, self.tx_policies.witness_limit()); | ||
policies.set(PolicyType::MaxFee, self.tx_policies.tip()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
policies.set(PolicyType::MaxFee, self.tx_policies.tip()); | |
policies.set(PolicyType::MaxFee, self.tx_policies.max_fee()); |
//if user set `script_gas_limit` we will use it's value only | ||
//if it is higher then the one estimated by assemble_tx | ||
if let Some(script_gas_limit) = self.script_gas_limit { | ||
if script_gas_limit > *tx.script_gas_limit() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really see a use case for that, the gas consumption doesn't fluctuate. If assemble_tx
estimated an amount of gas I don't see a usecase where we should submit more. Maybe I'm missing something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. users can consult the current block height and base their behavior off of that.
Or other transactions get included before this and change the contract state impacting script gas usage, etc.
If you assemble now, and don't get included in the next block then the script gas limit is no longer guaranteed to be correct.
not sure to what the node sets it to but if it is exactly what the dry run gave at that moment then we'll probably see users fallback to manually setting high values here just so they don't get flaky execution.
if script_gas_limit > *tx.script_gas_limit() { | ||
*tx.script_gas_limit_mut() = script_gas_limit; | ||
|
||
Self::set_max_fee_policy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line shouldn't be in the condition after of max_fee
and not on this one on script_gas_limit
?
_ => { | ||
return Err(error_transaction!( | ||
Builder, | ||
"`asseble_tx` did not return the right transactio type. Expected `create`" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for the other transaction type:
"`asseble_tx` did not return the right transactio type. Expected `create`" | |
"`assemble_tx` did not return the right transaction type. Expected `create`" |
self.witnesses.clone(), | ||
); | ||
|
||
if let Some(max_fee) = self.tx_policies.max_fee() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for others transaction type also : Is it necessary to set the max_fee
here because we try to edit it again after estimation can we change the current behavior : set -> override with assemble_tx -> maybe set again
to set with_assemble_tx -> maybe set again
and remove this
for c in contracts { | ||
self.log_decoder.merge(c.log_decoder()); | ||
} | ||
pub fn add_log_decoder(mut self, log_decoder: LogDecoder) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we document on that. What is a LogDecoder
could be interesting also because I don't see what is it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice :)) you managed to keep it very minimal
// As we have changed the witnesses and the predicate code depends on them we need | ||
// to estimate the predicates again before sending the tx. | ||
tx.estimate_predicates(&provider).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like something we should document somewhere -- that changing malleable fields requires re-estimation of the predicate if the predicate uses them in some way.
@@ -78,6 +83,11 @@ pub enum ScriptBuildStrategy { | |||
/// are present. Meant only for transactions that are to be dry-run with validations off. | |||
/// Useful for reading state with unfunded accounts. | |||
StateReadOnly, | |||
/// Transaction is estimated using `assemble_tx` and signatures are automatically added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about Strategy::Complete
vs Strategy::AssembleTx
.
Although assemble_tx
is a nice identifier internally between the devs (maybe even a label on the gql endpoints), I feel that we should have a better name.
All transactions are assembled in some way, only here we delegate the work to the node itself. Seeing as that is the key difference maybe:
LocalEstimation
vs RemoteEstimation
but that is also misleading since local is scarcely more than just querying N methods and then filling in the fields of the transaction. The decisions around what UTXOs to spend are still not owned by the SDK.
Having written the above I'm thinking: have we considered removing the SDKs ability to do the estimation automatically?
Leave in all the various helpers but have the strategy be called SignOnly
or something like that -- ie the user does the gas, contract, variable output and whatever else they want manually (potentially using sdk helpers), calling build
will just add the signatures over whatever is currently set in the builder?
async fn update_witnesses<T: UniqueIdentifier + Witnesses + Inputs>( | ||
tx: &mut T, | ||
unresolved_signers: &[Arc<dyn Signer + Send + Sync>], | ||
chain_id: &ChainId, | ||
) -> Result<()> { | ||
let id = tx.id(chain_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signers might be of different types, ie you cannot have a slice of &[AwsSigner, PrivateKeySigner, LedgerSigner].
S
is only going to resolve to one type at at time making the unresolved signers a slice of that particular signer type.
The builder has a add_signer(&mut self,...
that accepts a dyn Signer
so that is where the unresolved_signers
come from.
TLDR,
we chose type erasure for the tx builder so that we don't end up with ScriptTxBuilder<NextSigner<PrivateKeySigner, NextSigner<AwsSigner, LedgerSigner>>>
or something like that.
//if user set `script_gas_limit` we will use it's value only | ||
//if it is higher then the one estimated by assemble_tx | ||
if let Some(script_gas_limit) = self.script_gas_limit { | ||
if script_gas_limit > *tx.script_gas_limit() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. users can consult the current block height and base their behavior off of that.
Or other transactions get included before this and change the contract state impacting script gas usage, etc.
If you assemble now, and don't get included in the next block then the script gas limit is no longer guaranteed to be correct.
not sure to what the node sets it to but if it is exactly what the dry run gave at that moment then we'll probably see users fallback to manually setting high values here just so they don't get flaky execution.
if let Some(max_fee) = self.tx_policies.max_fee() { | ||
if max_fee > tx.max_fee_limit() { | ||
tx.policies_mut().set(PolicyType::MaxFee, Some(max_fee)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I think about it I'm leaning towards "if the user set something, then that is how it's going to be".
I could see it being a surprise if the user says spend at most 100 and we bump it to 100M because that is the way it was estimated.
If the user wants to add a bit of leeway to the estimations of assemble tx then that is maybe best done via separate tolerance args like we have for the max fee.
}) | ||
.collect() | ||
.enable_burn(true); // assemble tx will add missing change outputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. At what point do we check for burning? Ie if the assemble tx really adds missing change outputs, then can we do the check after the tx is assembled?
I don't doubt that it will add them but just that, IIUC, whenever users want to use assemble tx as the build strategy they have to say enable_burn
which sounds scary.
Co-authored-by: Mårten Blankfors <[email protected]> Co-authored-by: Ahmed Sagdati <[email protected]>
Co-authored-by: AurelienFT <[email protected]>
closes: #1641
Release notes
In this release, we:
Summary
We added support for assemble transactions and made it the default build strategy for all
CallHandlers
and transfers.Breaking Changes
Contract dependencies → automatic & log decoding
The old
with_contracts
/with_contract_ids
APIs are gone—contract inputs are now set using assemble tx, and if you need to decode logs/reverts you pass in the external contract’s decoder:Dependency & output estimation → fully automatic
You no longer need to chain on
with_variable_output_policy(...)
or calldetermine_missing_contracts()
: the SDK will auto-estimate both missing contracts and variable outputs.Script gas limit & TxPolicies → dedicated handler method
TxPolicies
does not hold thescript_gas_limit
anymore. Use.with_script_gas_limit(n)
directly on your call handler.Transaction submission →
submit
send_transaction
is renamed tosubmit
Manual fee/inputs adjustment → “assemble” strategy
You do not have to use
wallet.adjust_for_fee(&mut tb, …).await?
if you use the new assemble strategy.Removed the
ContractDependency
traitChecklist