search

getwax / bls-wallet

Core components to use layer 2 smart contract wallets with the BLS signature scheme
MIT License
177 stars 46 forks source link

567 bls provider and bls signer release #570

Closed JohnGuilding closed 1 year ago

JohnGuilding commented 1 year ago

What is this PR doing?

I've manually tested the new release against testnet and a local aggregator. I did this by adding the provider to the browser wallet and sending a tx to and from the wallet.

How can these changes be manually tested?

Does this PR resolve or contribute to any issues?

Resolves #567

Checklist

Guidelines

JohnGuilding commented 1 year ago

Note on testing:

I was not able to get transactions to work out of the box with the deployed aggregator OR a local aggregator. This was the case with just using the client module as normal, or the provider, so it wasn't an issue specific to the provider changes. Testing the client module by itself required new logic in the browser wallet to pay the aggregator (I haven't committed these changes but we should make this change at some point)

I can't say with confidence what the issue was with the deployed aggregator, as I'm not sure how recently that was updated. But the issue with the local aggregator was that the code failed in the AggregationStrategy.ts file. This if statement evaluated to true, and thus the bundle row never got added on this line.

I was able to get around the issue by manually updating this env var like so:

# old value - this value is provided in .env.example
MAX_GAS_PER_BUNDLE=2000000

# new value
MAX_GAS_PER_BUNDLE=8000000

Made the decision to publish these changes regardless as appears to be an issue with aggregator logic/aggregator config, as opposed to being related to an issue with the client module. If it is worth another issue to look into, then would deem it out of scope. Feel free to disagree!

Edit:

Some example values I was getting when checking why the bundle rows weren't getting added. These values correspond to values observed for newAggregatedGas:

Whereas this value should be under (according to the default config):

voltrevo commented 1 year ago

@JohnGuilding Hmm. MAX_GAS_PER_BUNDLE should be higher anyway, so all good with 8m.

However, that's A LOT of gas... could you provide some more detail on how you tested?

test provider on testnet with browser wallet (I've already done this)

I would have tested with an ETH transfer using Quill, but I can't tell from your description whether this is what you did.

Could you also confirm which network you used? testnet doesn't mean very much, because we've only ever used testnets so far. ie, do you mean geth-dev? hardhat? arbitrum-goerli?

JohnGuilding commented 1 year ago

@voltrevo yeah 8m does seem quite high.

could you provide some more detail on how you tested?