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: #60, Check if preimage exists on chain before creating a proposal #61

Merged
merged 3 commits into from
May 23, 2023
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
7 changes: 6 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
const { getConfiguration } = require("opstooling-js-style/src/eslint/configuration");

module.exports = getConfiguration({ typescript: { rootDir: __dirname } });
const conf = getConfiguration({ typescript: { rootDir: __dirname } });

conf.overrides[0].rules["@typescript-eslint/no-misused-promises"] = "off";
conf.overrides[0].rules["no-async-promise-executor"] = "off";

module.exports = conf;
9 changes: 8 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,14 @@ There are semi-automatic integration tests that execute the tip functions agains
| Local Kusama | `docker run --rm -p 9901:9901 parity/polkadot:v0.9.39 --chain=kusama-dev --tmp --alice --execution Native --ws-port 9901 --ws-external --force-kusama` |
| Local Polkadot | `docker run --rm -p 9900:9900 parity/polkadot:v0.9.39 --chain=dev --tmp --alice --execution Native --ws-port 9900 --ws-external` |

Note that the node needs to have the OpenGov features - Kusama development chain can be used for that (`--chain=kusama-dev`).
Notes:
- the node needs to have the OpenGov features - Kusama development chain can be used for that (`--chain=kusama-dev`).
- on macos apple silicon chip, you might see error `docker: no matching manifest for linux/arm64/v8 in the manifest list entries.`,
in this case
- close docker desktop,
- open terminal in Rosetta.
- run `open /Applications/Docker.app`
- add `--platform linux/amd64` flag to docker run command after `--rm`

With that, run the tests:

Expand Down
6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@
"test": "NODE_OPTIONS='--experimental-vm-modules --es-module-specifier-resolution=node' jest"
},
"dependencies": {
"@polkadot/api": "^10.3.2",
"@polkadot/util": "^11.0.1",
"@polkadot/util-crypto": "^11.0.1",
"@polkadot/api": "^10.7.1",
"@polkadot/util": "^12.2.1",
"@polkadot/util-crypto": "^12.2.1",
"opstooling-integrations": "https://github.com/paritytech/opstooling-integrations#v2.0.0",
"opstooling-js": "https://github.com/paritytech/opstooling-js#v0.0.14",
"probot": "^12.2.8",
Expand Down
91 changes: 63 additions & 28 deletions src/tip-opengov.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@ import "@polkadot/api-augment";
import "@polkadot/types-augment";
import { ApiPromise } from "@polkadot/api";
import { KeyringPair } from "@polkadot/keyring/types";
import { ISubmittableResult } from "@polkadot/types/types";
import { blake2AsHex } from "@polkadot/util-crypto";
import assert from "assert";
import { Probot } from "probot";

import { getChainConfig, getTipUrl } from "./chain-config";
import { State, TipRequest, TipResult } from "./types";
import { ContributorAccount, State, TipRequest, TipResult } from "./types";
import { formatReason, tipSizeToOpenGovTrack } from "./util";

export async function tipOpenGov(opts: {
Expand All @@ -29,40 +31,73 @@ export async function tipOpenGov(opts: {
if ("error" in track) {
return { success: false, errorMessage: track.error };
}
const contributorAddress = contributor.account.address;

const proposalTx = api.tx.utility.batch([
api.tx.system.remark(formatReason(tipRequest)),
api.tx.treasury.spend(track.value.toString(), contributor.account.address),
api.tx.treasury.spend(track.value.toString(), contributorAddress),
]);
const encodedProposal = proposalTx.method.toHex();
const proposalHash = blake2AsHex(encodedProposal);
const encodedLength = Math.ceil((encodedProposal.length - 2) / 2);
Copy link
Member

Choose a reason for hiding this comment

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

What is this calculation? It needs to be the SCALE encoded length.

Copy link
Contributor Author

@mordamax mordamax May 22, 2023

Choose a reason for hiding this comment

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

before it was proposalTx.length - 1, but this I took from https://github.com/polkadot-js/apps/blob/2d295e33f9d37d6582d97b9e93df81d16e1950e2/packages/page-preimages/src/Preimages/Add/Partial.tsx#L44

@ggwpez could you please explain what is SCALE encoded?

Copy link
Member

Choose a reason for hiding this comment

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

It first does toHex and then uses (x - 2) / 2 to get the number of bytes in the hex string… wtf
Maybe there is no easier way to encode it or something. Ideal would be an encode or encodedLength function.
Otherwise just add a test; i dont want to block over this if it works.

SCALE is the encoding that Substrate/Polkadot uses for mostly everything. Especially stuff like transactions and their arguments (so also preimages).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll investigate and we will add tests for sure


const preimage_unsub = await api.tx.preimage
.notePreimage(encodedProposal)
.signAndSend(botTipAccount, { nonce: -1 }, (result) => {
if (result.status.isInBlock) {
bot.log(`Preimage Upload included at blockHash ${result.status.asInBlock.toString()}`);
} else if (result.status.isFinalized) {
bot.log(`Preimage Upload finalized at blockHash ${result.status.asFinalized.toString()}`);
preimage_unsub();
}
});
return await new Promise(async (resolve, reject) => {
// create a preimage from opengov with the encodedProposal above
const preimageUnsubscribe = await api.tx.preimage
.notePreimage(encodedProposal)
.signAndSend(botTipAccount, { nonce: -1 }, async (result) => {
await signAndSendCallback(bot, contributor.account, "preimage", preimageUnsubscribe, result)
.then(async () => {
const readPreimage = await api.query.preimage.statusFor(proposalHash);

const referenda_unsub = await api.tx.referenda
.submit(
// TODO: There should be a way to set those types properly.
{ Origins: track.track } as any, // eslint-disable-line
{ Lookup: { hash: proposalHash, length: proposalTx.length - 1 } },
{ after: 10 } as any, // eslint-disable-line
)
.signAndSend(botTipAccount, { nonce: -1 }, (result) => {
if (result.status.isInBlock) {
bot.log(`Tip referendum included at blockHash ${result.status.asInBlock.toString()}`);
} else if (result.status.isFinalized) {
bot.log(`Tip referendum finalized at blockHash ${result.status.asFinalized.toString()}`);
referenda_unsub();
}
});
if (readPreimage.isEmpty) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Asking for understanding:
At this point, we made sure that the image submission transaction is isInBlock AND isFinalized.
How can it be that it's empty then?

Copy link
Contributor Author

@mordamax mordamax May 22, 2023

Choose a reason for hiding this comment

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

it can be, if account had insufficient amount of money to Reserve the ~1,3KSM , then the preimage will still be inBlock or Finalized, but not by fact be on-chain.

To Verify - send to bot ~0.1 UNIT, which will be enough for transaction fees, but not enough to Reserve
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
here i have 0,4 UNIT transferable, but for creating a preimage i will need 1,3

this is what happens with creating a preimage
image

image
In logs though all good, finalized, we proceeding to craetee a proposal. But there's no preimage. So it ends up with a proposal, without an "action" to execute after it's Voted and enaction time comes

Here's what happens with validation
image
image

reject(new Error(`Preimage for ${proposalHash} was not found, check if the bot has enough funds.`));
}

return { success: true, tipUrl: getTipUrl(contributor.account.network) };
const proposalUnsubscribe = await api.tx.referenda
.submit(
// TODO: There should be a way to set those types properly.
{ Origins: track.track } as never,
{ Lookup: { hash: proposalHash, len: encodedLength } },
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ Lookup: { hash: proposalHash, len: encodedLength } },
{ Lookup: { hash: proposalHash, len: readPreimage.len } },

Not sure if that is the correct syntax, but the len should be in the RequestStatus

image

You can also SCALE encode yourself or use the encoded len though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

len: readPreimage.len there's no such field in the returned response, there's readPreimage.encodedLength but it returns 54, while expected number is 85

Here's some logs

proposalTx.encodedLength 88
proposalTx.toString {"signature":{"signer":{"id":"CaKWz5omakTK7ovp4m3koXrHyHb7NG3Nt7GENHbviByZpKp"},"signature":{"ed25519":"0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"},"era":{"immortalEra":"0x00"},"nonce":0,"tip":0},"method":{"callIndex":"0x1800","args":{"calls":[{"callIndex":"0x0000","args":{"remark":"0x544f3a206d6f7264616d617820464f523a206d6567617265706f2331342028736d616c6c29"}},{"callIndex":"0x1203","args":{"amount":2000000000000,"beneficiary":{"id":"HnMAUz7r2G8G3hB27SYNyit5aJmh2a5P4eMdDtACtMFDbam"}}}]}}}
proposalTx.length 86
encodedProposal 0x180008000094544f3a206d6f7264616d617820464f523a206d6567617265706f2331342028736d616c6c2912030b00204aa9d10100e659a7a1628cdd93febc04a4e0646ea20e9f5f0ce097d9a05290d4a9e054df4e
encodedProposal.length 172
readPreimage.encodedLength 54 (¯\\_(ツ)_/¯)
encodedLength 85

when I create it, in polkadotjs it shows 85

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just added 5 chars to a remark, and got 90 as final size in polkadotjs

so it means to me

proposalTx.encodedLength - 3
proposalTx.length - 1
Math.ceil((encodedProposal.length - 2) / 2)

all 3 will give same correct size of preimage

https://github.com/search?q=+%22Math.ceil%28%28encodedProposal.length+-+2%29+%2F+2%29%3B%22&type=code

I just used last as this is most commonly used

Copy link
Member

Choose a reason for hiding this comment

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

Okay yea then just use what works. The TS code is fucked either way.

{ after: 10 } as never,
)
.signAndSend(botTipAccount, { nonce: -1 }, async (refResult) => {
await signAndSendCallback(bot, contributor.account, "referendum", proposalUnsubscribe, refResult)
.then(resolve)
.catch(reject);
});
})
.catch(reject);
});
});
}

async function signAndSendCallback(
bot: Probot,
contributor: ContributorAccount,
type: "preimage" | "referendum",
unsubscribe: () => void,
result: ISubmittableResult,
): Promise<TipResult> {
return await new Promise((resolve, reject) => {
if (result.status.isInBlock) {
bot.log(`${type} for ${contributor.address} included at blockHash ${result.status.asInBlock.toString()}`);
} else if (result.status.isFinalized) {
bot.log(`Tip for ${contributor.address} ${type} finalized at blockHash ${result.status.asFinalized.toString()}`);
unsubscribe();
resolve({ success: true, tipUrl: getTipUrl(contributor.network) });
} else if (
result.status.isDropped ||
result.status.isInvalid ||
result.status.isUsurped ||
result.status.isRetracted ||
result.status.isBroadcast
) {
const msg = `Tip for ${contributor.address} ${type} status is 👎: ${result.status.type}`;
bot.log(msg, result.status);
reject({ success: false, errorMessage: msg });
} else {
bot.log(`Tip for ${contributor.address} ${type} status: ${result.status.type}`, result.status);
}
});
}
2 changes: 1 addition & 1 deletion src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,4 @@ export type TipRequest = {
};
};

export type TipResult = { success: true; tipUrl: string } | { success: false; errorMessage?: string | undefined };
export type TipResult = { success: true; tipUrl: string } | { success: false; errorMessage?: string };
Loading