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

Fix operations execution for unrevealed accounts #1364

Merged
merged 1 commit into from
Jun 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ export const SignTransactionFormPage: React.FC<SignPageProps<FormValues>> = prop

<AdvancedSettingsAccordion />

<FormLabel>Approvers</FormLabel>
<FormLabel marginTop="24px">Approvers</FormLabel>
<Flex flexDirection="column" gap="12px" marginBottom="12px" data-testid="approvers">
{signers.map(signer => (
<AddressTile
Expand Down
2 changes: 1 addition & 1 deletion src/mocks/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,5 @@ export const addAccount = (account: Account | Multisig) => {
store.dispatch(accountsActions.addAccount(account));
};

export const fakeAddressExists = (revealedKeyPairs: { pkh: RawPkh }[]) => (pkh: RawPkh) =>
export const fakeIsAccountRevealed = (revealedKeyPairs: { pkh: RawPkh }[]) => (pkh: RawPkh) =>
Promise.resolve(revealedKeyPairs.map(keyPair => keyPair.pkh).includes(pkh));
3 changes: 3 additions & 0 deletions src/types/AccountOperations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ export type ImplicitOperations = {
export type AccountOperations = ProposalOperations | ImplicitOperations;
export type EstimatedAccountOperations = AccountOperations & {
estimates: Estimation[];
// for some operations taquito automatically adds a reveal operation
// and returns it as the first estimate
revealEstimate?: Estimation;
};

export const totalFee = (estimates: Estimation[]): number =>
Expand Down
12 changes: 6 additions & 6 deletions src/utils/hooks/setAccountDataHooks.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
mockSecretKeyAccount,
mockSocialAccount,
} from "../../mocks/factories";
import { addAccount, fakeAddressExists } from "../../mocks/helpers";
import { addAccount, fakeIsAccountRevealed } from "../../mocks/helpers";
import { mnemonic1 } from "../../mocks/mockMnemonic";
import { act, renderHook } from "../../mocks/testUtils";
import { ImplicitAccount, MnemonicAccount } from "../../types/Account";
Expand All @@ -36,7 +36,7 @@ describe("setAccountDataHooks", () => {
});

describe("mnemonic accounts", () => {
const addressExistsMock = jest.spyOn(tezosHelpers, "addressExists");
const isAccountRevealedMock = jest.spyOn(tezosHelpers, "isAccountRevealed");
const getFingerPrintMock = jest.spyOn(tezosHelpers, "getFingerPrint");
const encryptMock = jest.spyOn(functionsToMock, "encrypt");
const decryptMock = jest.spyOn(functionsToMock, "decrypt");
Expand Down Expand Up @@ -108,8 +108,8 @@ describe("setAccountDataHooks", () => {
await mnemonicAccount(4, `${LABEL_BASE} 5`),
await mnemonicAccount(5, `${LABEL_BASE} 6`),
];
addressExistsMock.mockImplementation(
fakeAddressExists(revealedAccounts.map(account => account.address))
isAccountRevealedMock.mockImplementation(
fakeIsAccountRevealed(revealedAccounts.map(account => account.address))
);

const {
Expand Down Expand Up @@ -148,8 +148,8 @@ describe("setAccountDataHooks", () => {
await mnemonicAccount(2, `${LABEL_BASE} 5`),
];
// Reveal mnemonic accounts
addressExistsMock.mockImplementation(
fakeAddressExists(expected.map(account => account.address))
isAccountRevealedMock.mockImplementation(
fakeIsAccountRevealed(expected.map(account => account.address))
);

const {
Expand Down
22 changes: 12 additions & 10 deletions src/utils/mnemonic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ import { networksActions } from "./redux/slices/networks";
import { store } from "./redux/store";
import * as tezosHelpers from "./tezos/helpers";
import { mockContractContact, mockImplicitContact, mockSocialAccount } from "../mocks/factories";
import { addAccount, fakeAddressExists } from "../mocks/helpers";
import { addAccount, fakeIsAccountRevealed } from "../mocks/helpers";
import { mnemonic1 } from "../mocks/mockMnemonic";
import { renderHook } from "../mocks/testUtils";
import { ImplicitAccount } from "../types/Account";
import { MAINNET } from "../types/Network";

const addressExistsMock = jest.spyOn(tezosHelpers, "addressExists");
const isAccountRevealedMock = jest.spyOn(tezosHelpers, "isAccountRevealed");

const testPublicKeys = [
{
Expand All @@ -37,7 +37,7 @@ beforeEach(() => {

describe("restoreRevealedPublicKeyPairs", () => {
it("restores existing accounts", async () => {
addressExistsMock.mockImplementation(fakeAddressExists(testPublicKeys));
isAccountRevealedMock.mockImplementation(fakeIsAccountRevealed(testPublicKeys));

const result = await restoreRevealedPublicKeyPairs(
mnemonic1,
Expand All @@ -49,7 +49,7 @@ describe("restoreRevealedPublicKeyPairs", () => {
});

it("restores first account if none exists", async () => {
addressExistsMock.mockImplementation(fakeAddressExists([]));
isAccountRevealedMock.mockImplementation(fakeIsAccountRevealed([]));

const result = await restoreRevealedPublicKeyPairs(
mnemonic1,
Expand All @@ -61,7 +61,9 @@ describe("restoreRevealedPublicKeyPairs", () => {
});

it("stops at first unrevealed account", async () => {
addressExistsMock.mockImplementation(fakeAddressExists([testPublicKeys[0], testPublicKeys[2]]));
isAccountRevealedMock.mockImplementation(
fakeIsAccountRevealed([testPublicKeys[0], testPublicKeys[2]])
);

const result = await restoreRevealedPublicKeyPairs(
mnemonic1,
Expand Down Expand Up @@ -109,8 +111,8 @@ describe("useRestoreRevealedMnemonicAccounts", () => {
derivationPathTemplate: "44'/1729'/?'/0'",
},
];
addressExistsMock.mockImplementation(
fakeAddressExists(expected.map(account => account.address))
isAccountRevealedMock.mockImplementation(
fakeIsAccountRevealed(expected.map(account => account.address))
);

const {
Expand All @@ -127,7 +129,7 @@ describe("useRestoreRevealedMnemonicAccounts", () => {
});

it("restores one account if none were revealed", async () => {
addressExistsMock.mockImplementation(fakeAddressExists([]));
isAccountRevealedMock.mockImplementation(fakeIsAccountRevealed([]));

const {
result: { current: restoreRevealedMnemonicsHook },
Expand All @@ -148,7 +150,7 @@ describe("useRestoreRevealedMnemonicAccounts", () => {
});

it("sets unique labels for restored accounts", async () => {
addressExistsMock.mockImplementation(fakeAddressExists(testPublicKeys.slice(0, 3)));
isAccountRevealedMock.mockImplementation(fakeIsAccountRevealed(testPublicKeys.slice(0, 3)));
store.dispatch(networksActions.setCurrent(MAINNET));
store.dispatch(contactsActions.upsert(mockImplicitContact(1, CUSTOM_LABEL)));
store.dispatch(contactsActions.upsert(mockContractContact(0, "ghostnet", `${CUSTOM_LABEL} 4`)));
Expand Down Expand Up @@ -180,7 +182,7 @@ describe("useRestoreRevealedMnemonicAccounts", () => {
});

it("restores existing accounts with a custom derivation path", async () => {
addressExistsMock.mockImplementation(fakeAddressExists(testPublicKeys.slice(0, 2)));
isAccountRevealedMock.mockImplementation(fakeIsAccountRevealed(testPublicKeys.slice(0, 2)));

const {
result: { current: restoreRevealedMnemonicsHook },
Expand Down
4 changes: 2 additions & 2 deletions src/utils/mnemonic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { generateMnemonic } from "bip39";
import { makeDerivationPath } from "./account/derivationPathUtils";
import { makeMnemonicAccount } from "./account/makeMnemonicAccount";
import { useGetNextAvailableAccountLabels } from "./hooks/labelsHooks";
import { PublicKeyPair, addressExists, derivePublicKeyPair, getFingerPrint } from "./tezos";
import { PublicKeyPair, derivePublicKeyPair, getFingerPrint, isAccountRevealed } from "./tezos";
import { MnemonicAccount } from "../types/Account";
import { Network } from "../types/Network";

Expand Down Expand Up @@ -44,7 +44,7 @@ export const restoreRevealedPublicKeyPairs = async (
mnemonic,
makeDerivationPath(derivationPathTemplate, accountIndex)
);
} while (await addressExists(pubKeyPair.pkh, network));
} while (await isAccountRevealed(pubKeyPair.pkh, network));
return result;
};

Expand Down
54 changes: 50 additions & 4 deletions src/utils/tezos/estimate.test.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import { estimate, handleTezError } from "./estimate";
import { addressExists, makeToolkit } from "./helpers";
import { isAccountRevealed, makeToolkit } from "./helpers";
import { executeParams } from "../../mocks/executeParams";
import { mockImplicitAccount, mockTezOperation } from "../../mocks/factories";
import { makeAccountOperations } from "../../types/AccountOperations";
import { GHOSTNET } from "../../types/Network";
jest.mock("./helpers", () => ({
...jest.requireActual("./helpers"),
makeToolkit: jest.fn(),
addressExists: jest.fn(),
isAccountRevealed: jest.fn(),
}));

const accountOperations = makeAccountOperations(mockImplicitAccount(1), mockImplicitAccount(1), [
Expand All @@ -17,7 +17,7 @@ const accountOperations = makeAccountOperations(mockImplicitAccount(1), mockImpl
describe("estimate", () => {
describe("Error handling", () => {
it("Catches unrevealed account", async () => {
jest.mocked(addressExists).mockResolvedValue(false);
jest.mocked(isAccountRevealed).mockResolvedValue(false);

jest.mocked(makeToolkit).mockImplementation(
() =>
Expand Down Expand Up @@ -51,7 +51,7 @@ describe("estimate", () => {
});
});

it("returns fee, storageLimit and gasLimit estimate ", async () => {
it("returns estimated operations", async () => {
const estimateResult = [
{
burnFeeMutez: 0,
Expand Down Expand Up @@ -84,4 +84,50 @@ describe("estimate", () => {

expect(result).toEqual(processedEstimateResult);
});

it("can deal with the reveal estimation", async () => {
const estimateResult = [
{
burnFeeMutez: 0,
consumedMilligas: 1,
gasLimit: 2,
minimalFeeMutez: 3,
operationFeeMutez: 4,
storageLimit: 5,
suggestedFeeMutez: 6,
totalCost: 7,
usingBaseFeeMutez: 8,
},
{
burnFeeMutez: 0,
consumedMilligas: 168681,
gasLimit: 169,
minimalFeeMutez: 269,
operationFeeMutez: 168.9,
storageLimit: 0,
suggestedFeeMutez: 289,
totalCost: 269,
usingBaseFeeMutez: 269,
},
];

const processedEstimateResult = {
...accountOperations,
estimates: [executeParams({ fee: 289, gasLimit: 169 })],
revealEstimate: executeParams({ fee: 7, gasLimit: 2, storageLimit: 5 }),
};

jest.mocked(makeToolkit).mockImplementation(
() =>
({
estimate: {
batch: jest.fn().mockResolvedValue(estimateResult),
},
}) as any
);

const result = await estimate(accountOperations, GHOSTNET);

expect(result).toEqual(processedEstimateResult);
});
});
44 changes: 33 additions & 11 deletions src/utils/tezos/estimate.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,19 @@
import { addressExists, makeToolkit, operationsToBatchParams } from "./helpers";
import { Estimate } from "@taquito/taquito";

import { isAccountRevealed, makeToolkit, operationsToBatchParams } from "./helpers";
import { Estimation } from "./types";
import { AccountOperations, EstimatedAccountOperations } from "../../types/AccountOperations";
import { Network } from "../../types/Network";

/**
* Estimates (and simulates the execution of) the operations.
*
* Note: if
*
* @param operations - operations to be estimated with a particular transaction signer
* @param network -
* @returns operations with their estimated fees, gas and storage limits
*/
export const estimate = async (
operations: AccountOperations,
network: Network
Expand All @@ -14,20 +26,20 @@ export const estimate = async (
try {
const estimates = await tezosToolkit.estimate.batch(operationsToBatchParams(operations));

let revealEstimate = undefined;
// taquito automatically adds a reveal operation (sometimes)
// if an account is not revealed yet
if (estimates.length > operations.operations.length) {
revealEstimate = estimateToEstimation(estimates.shift()!);
}

return {
...operations,
estimates: estimates.map(estimate => ({
storageLimit: estimate.storageLimit,
gasLimit: estimate.gasLimit,
fee: Math.max(estimate.suggestedFeeMutez, estimate.totalCost),
})),
estimates: estimates.map(estimateToEstimation),
revealEstimate,
};
// The way taquito works we need to take the max of suggestedFeeMutez and totalCost
// because the suggestedFeeMutez does not include the storage & execution cost
// and in these cases the totalCost is the one to go (so, for contract calls)
// though totalCost doesn't work well with simple tez transfers and suggestedFeeMutez is more accurate
} catch (err: any) {
const isRevealed = await addressExists(operations.signer.address.pkh, network);
const isRevealed = await isAccountRevealed(operations.signer.address.pkh, network);

if (!isRevealed) {
throw new Error(`Signer address is not revealed on the ${network.name}.`);
Expand All @@ -39,6 +51,16 @@ export const estimate = async (
}
};

const estimateToEstimation = (estimate: Estimate): Estimation => ({
storageLimit: estimate.storageLimit,
gasLimit: estimate.gasLimit,
// The way taquito works we need to take the max of suggestedFeeMutez and totalCost
// because the suggestedFeeMutez does not include the storage & execution cost
// and in these cases the totalCost is the one to go (so, for contract calls)
// though totalCost doesn't work well with simple tez transfers and suggestedFeeMutez is more accurate
fee: Math.max(estimate.suggestedFeeMutez, estimate.totalCost),
});

// Converts a known L1 error message to a more user-friendly one
export const handleTezError = (err: Error): string => {
if (err.message.includes("subtraction_underflow")) {
Expand Down
Loading
Loading