-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: added support for unknown txn types #3003
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.
Potentially I'm missing something but I'm not sure we need to treat this in the same way as other transaction types, with a request class and encoding. 🤔
packages/account/src/providers/transaction-request/unknown-transaction-request.ts
Outdated
Show resolved
Hide resolved
You make a good point re: enconding / decoding @danielbate so I agree with you there, but I disagree re: the request class. Here's my rationale (which I tried to allude to in the PR description) The impetus for this PR is for consumers sending transactions to have graceful failures, so therefore an For that reason, and to avoid breaking types in the responses that consumers work with, it needs to be treated as a Perhaps there may be another way to achieve the same result but I believe this most minimal way to do so, open to hearing your and other peoples thoughts. cc @arboleya @Torres-ssf |
@maschad, I believe a minimal solution for this issue would involve addressing three key methods: 1 - This is the most complex part. A user could potentially call it like this: const response = await TransactionReponse.create(idFromUnsupportedTx) We could add a try/catch block during the decoding to handle any errors. However, things become tricky afterward, as we can't simply throw the error—we still need to return a return a transaction result with TX summary, just like any other regular transaction. This might be where we need to introduce a export type TransactionUnsupported = {
type: TransactionType.Unsupported;
// TX Bytes
bytes: UInt8Array
}; This type might be necessary because the transaction property is required here. Most other properties can be either booleans (defaulting to false), empty arrays, or values extracted from the receipts. 2 - Since this method returns an array of transactions, we could simply log a warning and skip unsupported types, or alternatively, return them as 3 - This method returns a single transaction object. We could return a |
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.
Does this PR address the following:
New input or output types (i.e CoinInputV2)
New opcodes w/ brand new gas price in the consensus params
Or do we already handle these?
We call fuels-ts/packages/account/src/providers/transaction-request/unknown-transaction-request.ts Lines 51 to 57 in e7daa05
This performs fuels-ts/packages/account/src/providers/transaction-request/transaction-request.ts Lines 178 to 179 in e7daa05
We throw an error within these functions when the |
My apologies I should have been more explicit, the context of this PR is that should a consumer try to send or retrieve a transaction that is currently not supported that it will fail gracefully. Those methods handle this error that would be thrown as discussed here The only scenario someone could call |
After discussing with @Torres-ssf and @petertonysmith94 on a call, we decided it's best to remove the The PR has now been updated to handle the scenario where we are unable to decode a transaction hash when calling |
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 changes 👍🏻
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.
Clean! 🎯
Coverage Report:
Changed Files:
|
Release notes
In this release, we:
Summary
This PR introduces a
console.warn
for transaction types which the SDK may not support yet. I've made an assumption about what aBaseTransactionType
would contain as to distinguish it from a completely unsupported transaction.Checklist