Skip to content

🔨 deploy: sign safe proposals as proposer #826

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

Open
cruzdanilo wants to merge 2 commits into
base: main
Choose a base branch
from
Open

🔨 deploy: sign safe proposals as proposer #826

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
55cf997
🔨 deploy: sign safe proposals as proposer
cruzdanilo Oct 20, 2026
a5169e0
➖ dependencies: remove safe sdk
cruzdanilo Oct 20, 2026
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
94 changes: 64 additions & 30 deletions deploy/.utils/multisigPropose.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,47 +1,81 @@
import { env } from "process";
import { ethers, network, getNamedAccounts } from "hardhat";
import SafeApiKit from "@safe-global/api-kit";
import Safe from "@safe-global/protocol-kit";
import type { BaseContract } from "ethers";
import { TypedDataEncoder, ZeroAddress, type BaseContract } from "ethers";
import format from "./format";

const services: Record<string, string> = { 1: "eth", 10: "oeth", 8453: "base", 84532: "basesep" };
const types = {
SafeTx: [
{ name: "to", type: "address" },
{ name: "value", type: "uint256" },
{ name: "data", type: "bytes" },
{ name: "operation", type: "uint8" },
{ name: "safeTxGas", type: "uint256" },
{ name: "baseGas", type: "uint256" },
{ name: "gasPrice", type: "uint256" },
{ name: "gasToken", type: "address" },
{ name: "refundReceiver", type: "address" },
{ name: "nonce", type: "uint256" },
],
};

export default async (
account: string,
contract: BaseContract,
functionName: string,
args: readonly unknown[] = [],
safe = "multisig",
) => {
const { [account]: senderAddress, [safe]: safeAddress } = await getNamedAccounts();
const safeApiKit = new SafeApiKit({
chainId: BigInt(network.config.chainId ?? (await ethers.provider.getNetwork()).chainId),
});
const { [account]: sender, [safe]: safeAddress } = await getNamedAccounts();
const chainId = network.config.chainId ?? (await ethers.provider.getNetwork()).chainId;
const service = services[String(chainId)];
if (!service) throw new Error(`no safe transaction service for chain ${chainId}`);
const api = async <T>(path: string, body?: object) => {
const response = await fetch(`https://api.safe.global/tx-service/${service}/api/v1/safes/${safeAddress}/${path}`, {
method: body ? "POST" : "GET",
headers: {
"Content-Type": "application/json",
...(env.SAFE_API_KEY ? { Authorization: `Bearer ${env.SAFE_API_KEY}` } : {}),
},
body: body && JSON.stringify(body),
});
const text = await response.text();
if (!response.ok) throw new Error(text || response.statusText);
return (text && JSON.parse(text)) as T;
};
const to = await contract.getAddress();
const data = contract.interface.encodeFunctionData(functionName, args);
try {
if (
!(await safeApiKit.getPendingTransactions(safeAddress)).results.find((tx) => tx.to === to && tx.data === data)
) {
// eslint-disable-next-line no-console
console.log(`${safe}: proposing`, `${await format(to)}.${functionName}`, await format(args));
const safeSdk = await Safe.init({ safeAddress, signer: senderAddress, provider: network.provider });
if (!(await safeSdk.isOwner(senderAddress))) return printManual(safe, to, data);
const safeTransaction = await safeSdk.createTransaction({
transactions: [{ to, data, value: "0" }],
options: { nonce: await safeApiKit.getNextNonce(safeAddress) },
});
const safeTxHash = await safeSdk.getTransactionHash(safeTransaction);
const senderSignature = await safeSdk.signHash(safeTxHash);
await safeApiKit.proposeTransaction({
safeTxHash,
safeAddress,
senderAddress,
senderSignature: senderSignature.data,
safeTransactionData: safeTransaction.data,
origin: "deploy",
});
}
const { nonce } = await api<{ nonce: string }>("");
const { results } = await api<{ results: { to: string; data: string | null; nonce: number }[] }>(
`multisig-transactions/?executed=false&nonce__gte=${nonce}`,
);
Comment on lines +50 to +52

@chatgpt-codex-connector chatgpt-codex-connector Bot Oct 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Fetch the highest pending nonce before proposing

When this Safe has more pending transactions than the transaction service returns on its default page, results can omit the transaction with the highest nonce. The Math.max calculation then reuses an existing nonce and submits a competing proposal, rather than appending after all pending proposals as the previous getNextNonce call did. Request the highest nonce explicitly (for example, descending nonce ordering with a one-item limit) or follow pagination before calculating the new nonce.

Useful? React with 👍 / 👎.

@cruzdanilo cruzdanilo Oct 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This isn't a regression. The previous api-kit@2.4.6 getNextNonce also read only the first page of getPendingTransactions (no limit, so the service default of 100), and its duplicate check did the same. Hitting this would take more than 100 unexecuted proposals queued on the multisig at once. For scale, the optimism multisig has 117 transactions in its entire history, and its pending queue is empty right now. Handling it properly would mean paginating both the duplicate check and the nonce scan, so I'm leaving this as is unless the maintainer wants it.

Generated by Claude Code

if (results.some((tx) => tx.to === to && tx.data === data)) return;

// eslint-disable-next-line no-console
console.log(`${safe}: proposing`, `${await format(to)}.${functionName}`, await format(args));
const domain = { chainId, verifyingContract: safeAddress };
const safeTx = {
to,
value: 0,
data,
operation: 0,
safeTxGas: 0,
baseGas: 0,
gasPrice: 0,
gasToken: ZeroAddress,
refundReceiver: ZeroAddress,
nonce: Math.max(Number(nonce), ...results.map((tx) => Number(tx.nonce) + 1)),

@coderabbitai coderabbitai Bot Oct 20, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

sed -n '40,85p' deploy/.utils/multisigPropose.ts
rg -n 'multisigPropose\(|already queued|pending|results.some' deploy/.utils deploy test

Repository: exactly/protocol

Length of output: 2342

Fetch the full pending queue before both checks.

results contains only the current page. A matching queued call on a later page can bypass the duplicate check and be proposed again. A higher nonce on a later page can also cause the calculation to select an occupied nonce.

Fetch every page at the shared response boundary, then use the aggregated results for both the duplicate check and nonce calculation. ordering=-nonce is not sufficient because it does not inspect later pages for duplicate calls.

@cruzdanilo cruzdanilo Oct 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is the same point as the Codex thread above. Both checks read only the first page (100 pending transactions), and that's also how the api-kit getPendingTransactions/getNextNonce calls this replaces behaved. For it to matter, the multisig would need more than 100 unexecuted proposals queued at once. Its whole history has 117 transactions, and the queue is empty right now. I'm leaving it as is unless the maintainer wants pagination.

Generated by Claude Code

@coderabbitai coderabbitai Bot Oct 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@cruzdanilo Thanks for clarifying. Given the existing first-page behavior, I overstated this as a major issue introduced by this PR. Pagination remains an edge-case limitation, but I accept leaving it unchanged unless the maintainer requests it.

✏️ Learnings added 
Learnt from: cruzdanilo
Repo: exactly/protocol PR: 826
File: deploy/.utils/multisigPropose.ts:68-68
Timestamp: 2026-10-20T01:58:06.562Z
Learning: In exactly/protocol, deploy/.utils/multisigPropose.ts retains the prior Safe api-kit first-page behavior for pending transaction checks and nonce selection. The author considers full pagination optional unless the maintainer requests it. Do not describe this inherited limitation as a regression introduced by the direct Safe Transaction Service integration.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.

};
await api("multisig-transactions/", {
...safeTx,
contractTransactionHash: TypedDataEncoder.hash(domain, types, safeTx),
sender,
signature: await (await ethers.getSigner(sender)).signTypedData(domain, types, safeTx),
origin: "deploy",
});
} catch (error) {
if (senderAddress !== safeAddress) {
if (sender !== safeAddress) {
// eslint-disable-next-line no-console
console.log(`${safe}: error`, error instanceof Error ? error.message : error);
return printManual(safe, to, data);
Expand Down
3 changes: 0 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,6 @@
"@nomicfoundation/hardhat-ethers": "^3.0.8",
"@openzeppelin/hardhat-upgrades": "^3.2.1",
"@openzeppelin/upgrades-core": "^1.37.0",
"@safe-global/api-kit": "^2.4.5",
"@safe-global/protocol-kit": "^4.1.0",
"@safe-global/safe-core-sdk-types": "^5.1.0",
"@tenderly/hardhat-tenderly": "^2.3.0",
"@typechain/ethers-v6": "^0.5.1",
"@typechain/hardhat": "^9.1.0",
Expand Down
81 changes: 0 additions & 81 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading