Skip to content
This repository has been archived by the owner on Nov 5, 2023. It is now read-only.

551 run quill against geth node and pay aggregator fees #580

Merged
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
2 changes: 1 addition & 1 deletion contracts/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ For each network, the deployer contract can be deployed with the following scrip

To run integration tests:

1. cd into `./contracts` and run `yarn start-hardhat`
1. cd into `./contracts` and run `yarn start`
2. cd into `./aggregator` and run `./programs/aggregator.ts`
3. from `./contracts`, run `yarn test-integration`.

Expand Down
2 changes: 1 addition & 1 deletion contracts/clients/test/BlsSigner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe("BlsSigner", () => {
rpcUrl = "http://localhost:8545";
network = {
name: "localhost",
chainId: 0x7a69,
chainId: 0x539,
};

privateKey = await BlsSigner.getRandomBlsPrivateKey();
Expand Down
1 change: 1 addition & 0 deletions contracts/hardhat.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ const config: HardhatUserConfig = {
},
networks: {
hardhat: {
chainId: 1337,
Copy link
Collaborator Author

@JohnGuilding JohnGuilding Apr 19, 2023

Choose a reason for hiding this comment

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

Decided to enforce hardhat using the same chainId as the geth node. This means we don't have to change config when moving between the two. e.g. If you wanted to run quill against the hardhat node, you'd have to change the config file to use a different chainId.

Deemed changing hardhat chainId over changing the geth chainId mainly because it was easier. --dev mode for geth is not meant to be configured. If we wanted to change the chainId, looks like we'd have to run a private network with a custom genesis file which seems like a lot more effort than necessary.

Edit:
Another option could be to add an explicit hardhat local network and geth local network in the extension config files. Seen a similar pattern with wagmi where they offer config for different local chains (localhost, hardhat, foundry etc.). This option also seems like more work than is needed compared to just changing the hardhat chainId

initialBaseFeePerGas: 0, // workaround from https://github.com/sc-forks/solidity-coverage/issues/652#issuecomment-896330136 . Remove when that issue is closed.
accounts,
blockGasLimit: 30_000_000,
Expand Down
4 changes: 4 additions & 0 deletions docs/local_development.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,17 @@ yarn run dev:chrome # or dev:firefox, dev:opera

- pull latest from `main` and run the setup script from the root directory `./setup.ts`.
- Restart the node and redeploy contracts
- Delete `aggregator.sqlite` in `./aggregator`. This is the local DB which will get regenerated when the aggregator is started.
- Restart the aggregator and add the "-r" flag to the command e.g `./programs/aggregator.ts -r`.
- Reset the Quill extension in your browser if you're developing with Quill. You can do this by removing the extension and then re-adding via "Load unpacked" again. Or run `debug.reset();` twice in the background page console.

### Additional troubleshooting tips

- In general, the bundle or submission issues we've encountered have been us misconfiguring the data in the bundle or not configuring the aggregator properly.
- Be careful using Hardhat accounts 0 and 1 in your code when running a local aggregator. This is because the local aggregator config uses the same key pairs as Hardhat accounts 0 and 1 by default. You can get around this by not using accounts 0 and 1 elsewhere, or changing the default accounts that the aggregator uses locally.
- When packages are updated in the aggregator, you'll need to reload the deno cache as the setup script won't do this for you. You can do this with `deno cache -r deps.ts` in the `./aggregator` directory.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you'll need to reload the deno cache as the setup script won't do this for you

I think this statement is correct (reloading the cache being a necessity when packages have changed, rather than anything clever happening), correct me if anyone thinks that's wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nah it's not quite right. The cache logic only says it's ok to use a previous fetch of the same url. If the url is different, it'll refetch it. No cache clearing needed.

The problem is that a lot of urls in the dependency tree have semver-matching urls (e.g. @^1.0.0) instead of being pinned (e.g. @1.0.0). This can lead to some dependencies being more up-to-date than others, causing conflicts. There's also other ways urls can resolve to different things over time, like updates to esm.sh itself.

I wouldn't go into detail in the docs, instead just something like:

- Sometimes there are issues related to the deno cache. You can clear it with `deno cache -r deps.ts test/deps.ts` in the `./aggregator` directory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Forgot to reply to this. I see, that makes sense, thanks for pointing this out. Will update in the current work I'm doing

- If running Quill against a local node, and if you're using MetaMask to fund Quill, make sure the MetaMask
localhost network uses chainId `1337`.

### Tests

Expand Down
20 changes: 13 additions & 7 deletions extension/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,19 @@ Interaction with web3 applications can be slow, costly, and risky.
3. transfer L2 erc20 tokens
4. use contract wallet with L2 dapps (coming soon)

## Getting started

To run the dev server:

```sh
yarn run dev:chrome # or dev:firefox etc, see scripts in package.json
```

Reset the extension. This is useful for getting the wallet to a clean slate in development. You can achieve this in your browser by either:

- Removing the extension and then re-adding via "Load unpacked".
- Or run `debug.reset();` twice in the background page console.

## Feature Summary

### MVP (uses Optimism)
Expand Down Expand Up @@ -89,10 +102,3 @@ Interaction with web3 applications can be slow, costly, and risky.
- method name - nonce - (send)
- (aggregate) - (send all)

## Development

To run the dev server:

```sh
yarn run dev:chrome # or dev:firefox etc, see scripts in package.json
```
2 changes: 1 addition & 1 deletion extension/config.example.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
},
"local": {
"blockExplorerUrl": "N/A",
"chainId": "0x7a69",
"chainId": "0x539",
"displayName": "Local Network",
"logo": "",
"rpcTarget": "http://127.0.0.1:8545",
Expand Down
2 changes: 1 addition & 1 deletion extension/config.release.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
},
"local": {
"blockExplorerUrl": "N/A",
"chainId": "0x7a69",
"chainId": "0x539",
"displayName": "Local Network",
"logo": "",
"rpcTarget": "http://127.0.0.1:8545",
Expand Down
74 changes: 71 additions & 3 deletions extension/source/background/AggregatorController.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import { Aggregator, BlsWalletWrapper } from 'bls-wallet-clients';
import { ethers } from 'ethers';
import {
Aggregator,
BlsWalletWrapper,
AggregatorUtilities__factory as AggregatorUtilitiesFactory,
ActionData,
} from 'bls-wallet-clients';
import { BigNumber, ethers } from 'ethers';

import assert from '../helpers/assert';
import ensureType from '../helpers/ensureType';
Expand Down Expand Up @@ -71,12 +76,24 @@ export default class AggregatorController {
);

const nonce = (await wallet.Nonce()).toString();
const bundle = await wallet.sign({ nonce, actions });

const aggregatorUrl =
this.preferredAggregators[providerId] ?? network.aggregatorUrl;

const agg = new Aggregator(aggregatorUrl);

const actionsWithFeePaymentAction = await this.addFeePaymentAction(
nonce,
actions,
wallet,
agg,
);

const bundle = wallet.sign({
nonce,
actions: actionsWithFeePaymentAction,
});

const result = await agg.add(bundle);

assert(!('failures' in result), () => new Error(JSON.stringify(result)));
Expand Down Expand Up @@ -158,4 +175,55 @@ export default class AggregatorController {
this.preferredAggregators[providerId] = preferredAggregator;
},
});

async addFeePaymentAction(
nonce: string,
actions: Array<ActionData>,
wallet: BlsWalletWrapper,
aggregator: Aggregator,
) {
const network = await this.networkController.network.read();
const netCfg = getNetworkConfig(network, this.multiNetworkConfig);
const ethersProvider = await this.ethersProvider.read();

const aggregatorUtilitiesAddress = netCfg.addresses.utilities;
const aggregatorUtilitiesContract = AggregatorUtilitiesFactory.connect(
aggregatorUtilitiesAddress,
ethersProvider,
);

const actionsWithFeePaymentAction = [
...actions,
{
ethValue: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly a dumb question

Why is this action needed? And why do we set ethValue to 1?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nvm. Is this added here just to get an accurate fee estimate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why do we set ethValue to 1?

Not a dumb question at all, I had the exact same question before. I had a discussion with @voltrevo about this and it's possible to provide a value of 0 wei, or 1 wei for the ethValue when estimating fees. If you follow this thread from this comment onwards, that should provide some useful context.

My understanding is that you need to add the payment action to the estimate fee bundle in order to prevent the actual transaction costing more than the estimate. Then it is a choice whether the ethValue is 0 or 1. The discussion I've linked above contains some of the tradeoffs for each option.

contractAddress: aggregatorUtilitiesAddress,
encodedFunction:
aggregatorUtilitiesContract.interface.encodeFunctionData(
'sendEthToTxOrigin',
),
},
];

const feeEstimateBundle = wallet.sign({
nonce,
actions: [...actionsWithFeePaymentAction],
});

const feeEstimate = await aggregator.estimateFee(feeEstimateBundle);
const feeRequired = BigNumber.from(feeEstimate.feeRequired);
const safetyPremium = feeRequired.div(5);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think there would be a reason to change this safetyPremium semi frequently? Just wondering if we should make it an env variable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I think that's a good question. I originally took the value of 5 from an aggregator test, and used that in the provider. I'm not sure on the answer to the question tbh, and would be interested to get Andrews thoughts on this. I think another useful question would be is 20% to high as a safety margin? I've noticed the new signWithGasEstimate method in the contract-updates has a default value of 0, so after seeing that, a margin of 20% here seems very high.

@voltrevo do you see the need for a value like this to be changed ever?

And is a 20% margin even sensible in this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Decided to merge btw as wanted to add this change to merge-in-contract-updates, think this change would be useful to test quill with, as we want to enforce paying fees as the default behaviour.

Can add follow ups if needed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah 20% is quite high. I think it's fine for now so that we err on the side of reliability, but we'll definitely want to scrutinize this once compression delivers competitive pricing.

Some thoughts off the top of my head:

  • Add this to user configuration
  • Aggregator could estimate how much each account has over-paid and adjust accordingly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will add the safetyPremium to extension config.

Aggregator could estimate how much each account has over-paid and adjust accordingly

This sounds useful, worth creating an issue? Or just to be mindful of this when we circle back following compression work?

const safeFee = feeRequired.add(safetyPremium);

return [
...actions,
{
ethValue: safeFee,
contractAddress: aggregatorUtilitiesAddress,
encodedFunction:
aggregatorUtilitiesContract.interface.encodeFunctionData(
'sendEthToTxOrigin',
),
},
];
}
}