551 run quill against geth node and pay aggregator fees #580
Conversation
github-actions
bot
added
clients
contracts
| - 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Forgot to reply to this. I see, that makes sense, thanks for pointing this out. Will update in the current work I'm doing
|
|
||
| const feeEstimate = await aggregator.estimateFee(feeEstimateBundle); | ||
| const feeRequired = BigNumber.from(feeEstimate.feeRequired); | ||
| const safetyPremium = feeRequired.div(5); |
There was a problem hiding this comment.
Do you think there would be a reason to change this safetyPremium semi frequently? Just wondering if we should make it an env variable
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 actionsWithFeePaymentAction = [ | ||
| ...actions, | ||
| { | ||
| ethValue: 1, |
There was a problem hiding this comment.
Possibly a dumb question
Why is this action needed? And why do we set ethValue to 1?
There was a problem hiding this comment.
Oh nvm. Is this added here just to get an accurate fee estimate?
There was a problem hiding this comment.
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.
| @@ -131,6 +131,7 @@ const config: HardhatUserConfig = { | |||
| }, | |||
| networks: { | |||
| hardhat: { | |||
| chainId: 1337, | |||
There was a problem hiding this comment.
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
JohnGuilding
deleted the
551-run-quill-against-geth-node-and-pay-aggregator-fees
branch
What is this PR doing?
0x539(1337), from0x7a69(31337).How can these changes be manually tested?
yarn start, instead ofyarn start-hardhatDoes this PR resolve or contribute to any issues?
resolves #551
Checklist
Guidelines
resolve conversationbutton is for reviewers, not authors