Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

feat: add unlock all option to allow unknown accounts send transaction #3710

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open
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
19 changes: 14 additions & 5 deletions src/chains/ethereum/ethereum/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1949,13 +1949,22 @@ export default class EthereumApi implements Api {

const wallet = this.#wallet;
const isKnownAccount = wallet.knownAccounts.has(fromString);
const privateKey = wallet.unlockedAccounts.get(fromString);
let privateKey = wallet.unlockedAccounts.get(fromString);
satyajeetkolhapure marked this conversation as resolved.
Show resolved Hide resolved

if (privateKey === undefined) {
const msg = isKnownAccount
? "authentication needed: passphrase or unlock"
: "sender account not recognized";
throw new Error(msg);
// if unlock all option is set to true
// then create a fake private key for unknown account
if(this.#options.wallet.unlockAll)
{
privateKey = wallet.createFakePrivateKey(fromString)
}
Comment on lines +1957 to +1960
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be implemented in the wallet, rather than the api? I can imagine us chasing different cases where we need to unlock the account.

else
{
const msg = isKnownAccount
? "authentication needed: passphrase or unlock"
: "sender account not recognized";
throw new Error(msg);
}
Comment on lines +1957 to +1967
Copy link
Contributor

Choose a reason for hiding this comment

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

the styling here is not consistent with the code base - space after if, { should be on preceding line

}

await autofillDefaultTransactionValues(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import Wallet from "../../../src/wallet";
import { SECP256K1_N } from "@ganache/secp256k1";
import { Data, Quantity } from "@ganache/utils";

const ZERO_ADDRESS = "0x" + "0".repeat(40);

describe("api", () => {
describe("eth", () => {
describe("sendTransaction", () => {
Expand Down Expand Up @@ -223,7 +225,6 @@ describe("api", () => {

describe("unlocked accounts", () => {
it("can send transactions from an unlocked 0x0 address", async () => {
const ZERO_ADDRESS = "0x" + "0".repeat(40);
const provider = await getProvider({
miner: {
defaultGasPrice: 0
Expand Down Expand Up @@ -357,6 +358,83 @@ describe("api", () => {
assert(largeKey.pk < SECP256K1_N);
});
});

describe("unlock all accounts", async () => {
const providerOptions : EthereumProviderOptions = {
miner: {
defaultGasPrice: 0
},
chain: {
// use berlin here because we just want to test if we can use the
// "zero" address, and we do this by transferring value while
// setting the gasPrice to `0`. This isn't possible after the
// `london` hardfork currently, as we don't provide an option to
// allow for a 0 `maxFeePerGas` value.
// TODO: remove once we have a configurable `maxFeePerGas`
hardfork: "berlin"
}
}

it("can send transactions from an unlocked 0x0 address when unlock all is true", async () => {
providerOptions.wallet = {
unlockAll: true
}
const provider = await getProvider(
providerOptions
);
const [from] = await provider.send("eth_accounts");

await provider.send("eth_subscribe", ["newHeads"]);
const initialZeroBalance = "0x1234";
await provider.send("eth_sendTransaction", [
{ from: from, to: ZERO_ADDRESS, value: initialZeroBalance }
]);
await provider.once("message");
const initialBalance = await provider.send("eth_getBalance", [
ZERO_ADDRESS
]);
assert.strictEqual(
initialBalance,
initialZeroBalance,
"Zero address's balance isn't correct"
);
const removeValueFromZeroAmount = "0x123";
await provider.send("eth_sendTransaction", [
{ from: ZERO_ADDRESS, to: from, value: removeValueFromZeroAmount }
]);
await provider.once("message");
const afterSendBalance = BigInt(
await provider.send("eth_getBalance", [ZERO_ADDRESS])
);
assert.strictEqual(
BigInt(initialZeroBalance) - BigInt(removeValueFromZeroAmount),
afterSendBalance,
"Zero address's balance isn't correct"
);
});

it("cannot send transactions from an unlocked 0x0 address when unlock all is false", async () => {
providerOptions.wallet = {
unlockAll: false
}
const provider = await getProvider(
providerOptions
);
const [from] = await provider.send("eth_accounts");

await provider.send("eth_subscribe", ["newHeads"]);
const removeValueFromZeroAmount = "0x123";
const badSend = async () => {
return provider.send("eth_sendTransaction", [
{ from: ZERO_ADDRESS, to: from, value: removeValueFromZeroAmount }
]);
};
await assert.rejects(
badSend,
"Error: sender account not recognized"
);
});
});
});
});
});
20 changes: 19 additions & 1 deletion src/chains/ethereum/options/src/wallet-options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,16 @@ export type WalletConfig = {
hd_path: string;
};
};

/**
* Unlock all accounts irrespective of known or unknown status.
*
* @defaultValue false
*/
unlockAll: {
type: boolean;
hasDefault: true;
};
};
exclusiveGroups: [
["accounts", "totalAccounts"],
Expand Down Expand Up @@ -326,5 +336,13 @@ export const WalletOptions: Definitions<WalletConfig> = {
default: () => ["m", "44'", "60'", "0'", "0"],
legacyName: "hd_path",
cliType: "string"
}
},
unlockAll: {
normalize,
cliDescription:
"Unlock all accounts irrespective of known or unknown status.",
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of better/clearer alternatives, but I wonder if this help text will make sense to our users.

I'm curious on other reviewers' thoughts on this

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree - the term "known or unknown status" seems to be an internal concern. We could probably get more information in there, but at the very least, just "Unlock all accounts" would be clearer.

default: () => false,
cliAliases: ["unlockAll"],
cliType: "boolean"
},
};