Skip to content
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: enhance the communication between SDK and wallet #3735

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

petertonysmith94
Copy link
Contributor

@petertonysmith94 petertonysmith94 commented Feb 24, 2025

Release notes

In this release, we:

  • Enhanced the SDK to pass additional information through to the underlying connectors to reduce HTTP requests.

Summary

  • In this change, we have added the following to the SendTransactionJsonRpc specification:
interface SendTransactionJsonRpc extends ExistingSendTransactionJsonRpc {
  // ...
  provider: {
    url: string,
    cache?: {
      consensusParameterTimestamp?: string,
      chain: ChainInfoJson,
      nodeInfo: NodeInfoJson,
    }
  },
  data?: {
    transactionSummary?: TransactionSummaryJson;
    estimatedGasPrice?: string; // Remove
    latestGasPrice?: string; // Remove
  },
  state: undefined | 'funded' | 'signed',
}
  • Definitions of the transaction request state:
State Description
undefined The transaction has not been estimated or funded.
funded The transaction is estimated and funded, the underlying wallet can skip this process.
signed The transaction is signed (therefore, estimated and funded). This is the same as skipCustomFee.
  • We can now re-instantiate a Provider using the serialised cache:
function sendTransaction(request: TransactionRequest, { provider: { url, cache } }) {
  // This will then not perform a request to the node for chain + node info
  const provider = new Provider(url, { cache });
  
  // ...
}
  • We can now construct a transaction summary from it's serialized form:
const transactionSummary: TransactionSummaryJson = {
  id: request.getTransactionId(chainId),
  transactionBytes: request.toTransactionBytes(),
  gasPrice: txCost.gasPrice.toString(),
  receipts: txCost.rawReceipts,
};
const assembledSummary = await assembleTransactionSummaryFromJson({
  provider,
  transactionSummary,
});

Checklist

  • All changes are covered by tests (or not applicable)
  • All changes are documented (or not applicable)
  • I reviewed the entire PR myself (preferably, on GH UI)
  • I described all Breaking Changes (or there's none)

* feat: implement flags for the transaction request

* chore: combine flags for `isEstimated` and `isFunded`

* chore: remove `isSigned` from witness updates

* changeset

* revert experimental changes

* Aligned to the new re-spec

* update to state

* Update state

* chore: fix connector test

* lintfix

* Apply suggestions from code review
@petertonysmith94 petertonysmith94 added the feat Issue is a feature label Feb 24, 2025
@petertonysmith94 petertonysmith94 self-assigned this Feb 24, 2025
Copy link

vercel bot commented Feb 24, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fuels-template ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 28, 2025 11:54am
ts-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 28, 2025 11:54am
ts-docs-api ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 28, 2025 11:54am

@petertonysmith94 petertonysmith94 changed the title feat: added the state to the transaction request (#3709) feat: enhanced the SDK and wallet communication Feb 24, 2025
@petertonysmith94 petertonysmith94 changed the title feat: enhanced the SDK and wallet communication feat: enhance the communication between SDK and wallet Feb 24, 2025
* chore: moved chainInfo serialization

* chore: added serialize chain info

* move chain info helper

* add deprecation notice

* chore: added deserialize of nodeInfo

* Added the ability to restore the provider chain + node info cache

* consolidate serialization

* serialize provider cache

* pass serialized chain to connector

* lintfix

* lintfix

* changeset

* changeset

* lintfix
Comment on lines 754 to 758
if (params.skipCustomFee) {
const chainId = await this.provider.getChainId();
request.updateState(chainId, 'signed');
return { request, state: 'signed' };
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could there be cases where the skipCustomFee flag is used, but the transaction is not yet signed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that could be a possibility.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On top of the above, I also think a method named validate should not mutate its received parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On top of the above, I also think a method named validate should not mutate its received parameters.

4a3e21e

Comment on lines 55 to 68
export type FuelConnectorSendTxParams = {
skipCustomFee?: boolean;
onBeforeSend?: (txRequest: TransactionRequest) => Promise<TransactionRequest>;
provider?: {
url: string;
cache?: SerializedProviderCache;
};
data?: {
transactionSummary?: SerializedTransactionSummary;
estimatedGasPrice?: string;
latestGasPrice?: string;
};
state?: TransactionStateFlag['state'];
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed that the interface name being used here differs from the one being specified within this PR's description. Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the SerializedProviderCache?

I wanted to show the full payload in the description so expanded the SerializedProviderCache to get:

  provider: {
    url: string,
    cache?: {
      consensusParameterTimestamp?: string,
      chain: SerializedChainInfo,
      nodeInfo: SerializedNodeInfo,
    }
  },

Copy link
Contributor

@nedsalk nedsalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just commented some nits.

arboleya

This comment was marked as duplicate.

Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
77.31%(+0.18%) 70.96%(+0.11%) 75.46%(+0.23%) 77.31%(+0.18%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/account/src/account.ts 82.72%
(+1.69%)
73.17%
(+1.17%)
82.92%
(+0.42%)
82.47%
(+1.68%)
🔴 packages/account/src/providers/provider.ts 68.24%
(+0.71%)
60.18%
(+0.38%)
68.75%
(+0.71%)
67.99%
(+0.7%)
🔴 packages/account/src/providers/generated/operations.ts 94.83%
(+1.29%)
100%
(+0%)
84.31%
(+3.92%)
95.23%
(+1.19%)
🔴 packages/account/src/providers/transaction-request/transaction-request.ts 88.96%
(+0.39%)
78.66%
(+0.58%)
84.31%
(+0.31%)
89.18%
(+0.37%)
✨ packages/account/src/providers/transaction-summary/assemble-transaction-summary-from-serialized.ts 100%
(+100%)
100%
(+100%)
100%
(+100%)
100%
(+100%)
🔴 packages/account/src/providers/transaction-summary/receipt.ts 92.85%
(-7.15%)
100%
(+0%)
80%
(-20%)
92.85%
(-7.15%)
packages/account/src/providers/transaction-summary/status.ts 100%
(+0%)
100%
(+6.67%)
100%
(+0%)
100%
(+0%)
🔴 packages/account/src/providers/utils/receipts.ts 84.61%
(-10.7%)
50%
(-22%)
100%
(+0%)
84.61%
(-10.77%)
🔴 ✨ packages/account/src/providers/utils/serialization.ts 98.57%
(+98.57%)
76.19%
(+76.19%)
100%
(+100%)
98.63%
(+98.63%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Issue is a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve SDK and Wallet Communication
4 participants