feat: status flags for the transaction request #3709
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
petertonysmith94
changed the title
|
Coverage Report:
Changed Files:
|
petertonysmith94
requested review from
digorithm,
arboleya,
Torres-ssf,
danielbate and
nedsalk
as code owners
| @@ -87,6 +87,8 @@ export class ScriptTransactionRequest extends BaseTransactionRequest { | |||
|
|
|||
| await account.fund(this, txCost); | |||
|
|
|||
| this.updateFlags({ isEstimated: true, isFunded: true }); | |||
There was a problem hiding this comment.
Is there a case where these two can exist in isolation, and if not, should we merge them into a single flag?
There was a problem hiding this comment.
My initial solution was to have the isEstimate triggered using an estimate method, then the isFunded on the fund method.
@danielbate suggested that we only update in the estimateAndFund method for now as this is the primary suggested entry point for estimate + funding.
Given that users can estimate and fund with other avenues, I believe keeping these flags separate gives us more flexibility to enhance this feature later without external changes.
There was a problem hiding this comment.
How do we expect the Wallet to behave in these two scenarios?
// 1
this.updateFlags({ isEstimated: true, isFunded: false });
// 2
this.updateFlags({ isEstimated: false, isFunded: true });
There was a problem hiding this comment.
{ isEstimated: true, isFunded: false} - wallet does not dry-run the transaction on page load. It also expects the gasLimit and maxFee to be set correctly. Based on this, the wallet will fetch resources and add them to the request.
{ isEstimated: false, isFunded: true } - wallet increases gasLimit by 20% and dry-runs the transaction. It assumes funding is correct but IF the prior change has increased the maxFee, we would need to re-fund.
As @nedsalk suggested in sync, I'm wondering whether a status enum would be more flexible. This would make it more resistant to any changes to dry-run that could allow it to also introduce funding.
There was a problem hiding this comment.
Asking from another angle: how could a transaction be funded and not estimated?
There was a problem hiding this comment.
I'm missing something here as well. Why is the gasLimit increased by 20% for a dry run when it was never accurately estimated in the first place?
I'm trying to understand if this is meant to address a specific corner case.
There was a problem hiding this comment.
The wallet makes an assumption that the gasLimit is not set high enough: https://github.com/FuelLabs/fuels-wallet/blob/d5721074c139a5a257cfc95e7ec67f29b4d24e16/packages/app/src/systems/Transaction/services/transaction.tsx#L233
I think we need to make the spec clearer so it is obvious for people what is happening and when, and we will run this by the front-end team.
There was a problem hiding this comment.
I apologize for repeating myself, but just to be sure my question haven't gone unnoticed:
- How could a transaction be funded and not estimated?
There was a problem hiding this comment.
Apologies I wanted to think about this a little more - a transaction could not be funded and not estimated. We're going to rethink approach a little and will be making some changes to the spec.
| * @param flags - The flags to update. | ||
| */ | ||
| public updateFlags(flags: Partial<{ [keys in keyof TransactionStatusFlags]: boolean }>) { | ||
| const CHAIN_ID = 0; |
There was a problem hiding this comment.
Is this supposed to be hardcoded?
There was a problem hiding this comment.
Yes, these flags will only serve as a checksum hash to ensure the transaction body hasn't changed between steps.
By hardcoding the CHAIN_ID, we avoid having to either pass through a chain ID or making the async.
There was a problem hiding this comment.
I may be missing the point or how a corner case like this could happen, but this seems like it could lead to future problems. Was this agreed upon with @FuelLabs/frontend?
There was a problem hiding this comment.
I agree with @arboleya. Maybe I'm missing something, but since all methods calling updateFlags are async, what’s the benefit of making this method synchronous?
Also, provider.getChain() will usually resolve immediately since it relies on cached data.
There was a problem hiding this comment.
By storing the hash, it enables the detection of changes like the following:
const request = new ScriptTransactionRequest({ });
request.estimateAndFund(wallet);
// Sub-sequential changes to the request wo/ estimating or funding can be detected
request.addResources(...)The consumer of these flags (the wallet), can choose whether they use these flags or not:
// Simple if statement
if (!request.flags.isEstimated)
// Or checking the request hash
const requestHash = request.getTransactionId(0);
if (request.flags.isEstimated !== requestHash)Happy to change these flags to simple boolean values, but I believe having a means of detecting changes post estimate and fund would benefit this process.
There was no agreement with the @FuelLabs/frontend team, but it is in the proposed solution.
There was a problem hiding this comment.
I can appreciate the confusion with using a different chain ID, I think it makes sense to use the actual value.
Summary
Introduction of flags to the transaction request which get calculated at strategic steps:
isEstimatedisFundedisSigned(leverages the existingskipCustomFeeparameter)This will enable connectors to skip certain steps based upon these flags to reduce HTTP requests.
Checklist