-
Notifications
You must be signed in to change notification settings - Fork 46
Conversation
b1da6c7
to
e42ccf2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Code all looks good, so gonna merge. Just had some questions for my clarity
const compressedBundle = await this.bundleCompressor.compress(bundle); | ||
|
||
const [rawTx, rawCompressedTx] = await Promise.all([ | ||
this.verificationGateway.populateTransaction.processBundle(bundle).then( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to populate the transaction with the VG and also with the bundle compressor? Or do we just do the VG one so that we can log out what the rawTx
length is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah it's only for logging.
); | ||
|
||
await receiptOf( | ||
blsPublicKeyRegistry.register( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for my understanding - We register a wallet with the registry because it allows us to compress the transaction even more. Is this the only way we can currently register a wallet? Asking just to check if a wallet would get registered on its own after its first transaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty much yeah. It doesn't happen automatically. We need to do something about that.
Also:
it allows us to compress the transaction even more
technically correct but I wouldn't phrase it that way. It's not 'even better', it's essential - otherwise your tx will be more expensive, not less.
Dependent PR
This PR depends on #586.
After that's merged, toggle the target branch back and forth or push an empty commit to fix the diff. (Preview of this PR's changes.)
(If you want, you can also just review+merge this one directly. Github will automerge the previous PRs.)
What is this PR doing?
Uses compression in aggregator. Reduces the
data
field when sending a single ETH transfer from 836 bytes down to 164 bytes.How can these changes be manually tested?
(Replicating this test isn't required.)
Use Quill to do an ETH transfer and verify the transaction data is about 324 bytes. Here's what I got on arbitrum goerli:
Explorer link
To reduce this further, use
./manualTests/registerWallet.ts
to register the source BLS key and address, as well as the destination address. Verify that reduces the transaction data down to about 164 bytes:Explorer link
For comparison I used version
f8a8c490
(git checkout f8a8c490
) (this is right before thecontract-updates
merge, which naturally changes the contracts somain
currently doesn't work with the existing deployment). Here's the transaction data from that one:Explorer link
Wait, wait, still 164 bytes? 😰
Yeah. That's just the data field too. If you include the full transaction data (including the envelope with the aggregator's signature and gas settings etc), then there's another 115 bytes or so (total is about 279 bytes).
Anyway, 164 bytes in the data field is a lot more than we need, and it's because there's still some cleanup to do. Here's the breakdown of those bytes:
In summary:
We should be able to remove the ABI overhead by introducing another contract whose whole purpose is to call the expander with its raw argument.
Focusing on the positive, very small encodings for the actual operations have been achieved, and this means that the marginal data cost of each operation is this low number (20 bytes or so). This means you could add 100 transactions for 2100 bytes, so the whole transaction would be 2100+279=2379 bytes, or about 24 bytes per transaction. Then the marginal cost becomes dominant, BUT you need to wait for 100 transactions before you can submit.
Related improvements to work on:
Does this PR resolve or contribute to any issues?
Resolves #406.
Checklist
Guidelines
resolve conversation
button is for reviewers, not authors