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.

Sign up for GitHub

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

Jump to bottom

LIVE-12087 - Fix llm prepare sign tx #6759

Closed
wants to merge 2 commits into from
Closed

LIVE-12087 - Fix llm prepare sign tx #6759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ab7bab
fix(livesdklogic): adjust gas setting to be on par with lld
ComradeAERGO Apr 26, 2024
798b8cc
fix(tests): fixed tests for livesdklogic preparesigntx
ComradeAERGO Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/old-teachers-march.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"live-mobile": minor
---

Standardize PrepareSignTx behavior\

33 changes: 19 additions & 14 deletions apps/ledger-live-mobile/src/components/Web3AppWebview/liveSDKLogic.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,15 @@ describe("prepareSignTransaction", () => {
// Given
const parentAccount = createAccount("12");
const childAccount = createTokenAccount("22", "js:2:ethereum:0x012:");
const expectedResult: EvmTransaction = {
const expectedResult: Partial<EvmTransaction> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure that testing half of the expected result is enough.
The function is expected to return a full Transaction object, not a Partial one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what the prepareSignTransaction returns

Copy link
Contributor

Choose a reason for hiding this comment

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

It feels a bit wrong to expect a partial transaction now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what the prepareSignTransaction returns

amount: new BigNumber("1000"),
data: Buffer.from([]),
family: "evm",
feesStrategy: "medium",
gasPrice: new BigNumber("700000"),
gasLimit: new BigNumber("1200000"),
customGasLimit: new BigNumber("1200000"),
mode: "send",
gasPrice: new BigNumber("700000"),
nonce: 8,
recipient: "0x0123456",
subAccountId: "js:2:ethereum:0x022:",
useAllAmount: false,
maxFeePerGas: undefined,
maxPriorityFeePerGas: undefined,
type: 1,
chainId: 0,
};

// When
Expand All @@ -49,7 +41,11 @@ describe("prepareSignTransaction", () => {
});

// *** UTIL FUNCTIONS ***
function createEtherumTransaction(): Partial<Transaction & { gasLimit: BigNumber }> {
function createEtherumTransaction(): Partial<
Transaction & {
gasLimit: BigNumber;
}
> {
return {
family: "evm",
amount: new BigNumber("1000"),
Expand Down Expand Up @@ -149,9 +145,18 @@ function createTokenAccount(id = "32", parentId = "whatever"): TokenAccount {
pendingOperations: [],
starred: false,
balanceHistoryCache: {
WEEK: { latestDate: null, balances: [] },
HOUR: { latestDate: null, balances: [] },
DAY: { latestDate: null, balances: [] },
WEEK: {
latestDate: null,
balances: [],
},
HOUR: {
latestDate: null,
balances: [],
},
DAY: {
latestDate: null,
balances: [],
},
},
swapHistory: [],
};
Expand Down
21 changes: 7 additions & 14 deletions apps/ledger-live-mobile/src/components/Web3AppWebview/liveSDKLogic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,15 @@ import BigNumber from "bignumber.js";

export default function prepareSignTransaction(
account: AccountLike,
parentAccount: Account | undefined,
liveTx: Partial<Transaction & { gasLimit: BigNumber }>,
parentAccount: Account | null | undefined,
liveTx: Partial<
Transaction & {
gasLimit: BigNumber;
}
>,
): TransactionCommon {
const bridge = getAccountBridge(account, parentAccount);
const t = bridge.createTransaction(account);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it okay to not go through the createTransaction anymore ?
I understand you made it the same as LLD but I think that some special defaults could be setup in this function IIRC

const { recipient, ...txData } = liveTx;
const t2 = bridge.updateTransaction(t, {
recipient,
return bridge.updateTransaction(liveTx, {
subAccountId: isTokenAccount(account) ? account.id : undefined,
});

return bridge.updateTransaction(t2, {
customGasLimit: txData.gasLimit,
type: 1,
maxFeePerGas: undefined,
maxPriorityFeePerGas: undefined,
...txData,
});
}