Update values returned in bundle receipt to more closely match ethers transaction response

JohnGuilding commented 1 year ago

We should update the bundle receipt to return additional information about the transaction. This means updating the lookupReceipt() method in the BundleService.

The bundle receipt currently looks like this:

export type BundleReceipt = {
  transactionIndex: string;
  transactionHash: string;
  bundleHash: string;
  blockHash: string;
  blockNumber: string;
};

As part of the provider work, we need to return an ethers transaction receipt that looks like this:

export interface TransactionReceipt {
  to: string;
  from: string;
  contractAddress: string,
  transactionIndex: number,
  root?: string,
  gasUsed: BigNumber,
  logsBloom: string,
  blockHash: string,
  transactionHash: string,
  logs: Array<Log>,
  blockNumber: number,
  confirmations: number,
  cumulativeGasUsed: BigNumber,
  effectiveGasPrice: BigNumber,
  byzantium: boolean,
  type: number;
  status?: number
};

The transaction receipt information is mostly derived from the bundle receipt, but there a several pieces of information missing. We can improve this by updating the lookupReceipt() method in the BundleService.

As part of the BLS provider/signer work, we have overridden the ethers getTransactionReceipt() method which calls _getTransactionReceipt(). A developer would use this either by explicitly calling provider.getTransactionReceipt(), or more commonly, by calling it implicitly with await tx.wait() after sending a transaction.

We should update _getTransactionReceipt() so that it returns a more accurate transaction receipt in the BlsProvider.

Note: There are a few other TODOs that are related to this issue that should be completed as part of this work. In bls-wallet, search for "// TODO: bls-wallet #412" to find them

Acceptance criteria

The BundleReceipt type returned from lookupReceipt() in ./clients/src/Aggregator.ts has been updated to include values from the ethers TransactionReceipt.
The lookupReceipt() method at ./aggregator/src/app/BundleService.ts returns the values needed for the lookupReceipt() function in ./clients/src/Aggregator.ts.
```
// e.g.
return {
bundleHash: hash,
to: receipt.to,
from: receipt.from,
...
```
Add test for lookupReceipt() in BundleServiceSubmitting.test.ts
The BlsProvider _getTransactionReceipt() method has been updated to return the updated bundle receipt
BlsProvider tests updated (Should be able to modify existing tests rather than create new cases)
Remove TODO: bls-wallet #412 comments, replace with updated ones if needed.
~The eth_getTransactionReceipt rpc method should be updated in extension/src/backgroundAggregatorController.ts~

ri-dev-git commented 1 year ago

Hey there! I am a beginner in web3 space and would like to try to fix this issue.

JohnGuilding commented 1 year ago

Hey @ri-dev-git - welcome! That's great to hear.

How much dev experience do you have?

First, take a read over our guidelines on contributing. You'll need to assign yourself to this issue and set up your local dev environment in a fork of our repo.

You'll then need to create a branch, make the changes, update tests, and then raise a pull request.

Let me know once you've set up your development environment, and we can go into further detail on the changes! Please shout if you have any questions.

FYI we're aiming to get this change in by mid January, which is plenty of time dw :)

ri-dev-git commented 1 year ago

I have been full stack dev for ast 1 year and started my blockchain journey in last two months. Excited to work the issue. I will connect if i face some issue during development! Thank you for your help! @JohnGuilding 😊

ri-dev-git commented 1 year ago

Hey there @JohnGuilding I have set up the development env.

ri-dev-git commented 1 year ago

Hey there @JohnGuilding! I have a confusion does the BunndleReceipt need to be updated w/ transactionResponse or transactionReciept's Response?

What I have done is:

export type BundleReceipt = {
  transactionIndex: number;
  transactionHash: string;
  bundleHash: string;
  blockHash: string;
  blockNumber: number;

  confirmations: number,
  from: string;
  raw?: string

  wait: (confirmations?: number) => Promise<TransactionReceipt>
};

export interface Log {
  blockNumber: number;
  blockHash: string;
  transactionIndex: number;

  removed: boolean;

  address: string;
  data: string;

  topics: Array<string>;

  transactionHash: string;
  logIndex: number;
}

export interface TransactionReceipt {
  to: string;
  from: string;
  contractAddress: string,
  transactionIndex: number,
  root?: string,
  gasUsed: BigNumber,
  logsBloom: string,
  blockHash: string,
  transactionHash: string,
  logs: Array<Log>,
  blockNumber: number,
  confirmations: number,
  cumulativeGasUsed: BigNumber,
  effectiveGasPrice: BigNumber,
  byzantium: boolean,
  type: number;
  status?: number
};

Am i on right track?

JohnGuilding commented 1 year ago

Hey @ri-dev-git,

The aim is to add the values from the ethers TransactionReceipt to our BundleReceipt.

Something like this:

export type BundleReceipt = {
  bundleHash: string;
  to: string;
  from: string;
  contractAddress: string;
  transactionIndex: number;
  root?: string,
  gasUsed: BigNumber;
  logsBloom: string;
  blockHash: string;
  transactionHash: string;
  logs: Array<ethers.providers.Log>;
  blockNumber: number;
  confirmations: number;
  cumualtiveGasUsed: BigNumber;
  effectiveGasUsed: BigNumber;
  byzantium: boolean;
  type: number;
  status?: number
};

JohnGuilding commented 1 year ago

@ri-dev-git I've updated the description of the issue with some acceptance criteria that will clarify the work that needs to be done.

ri-dev-git commented 1 year ago

Yes! Thank you! I had one question: Do I need to send the bundleHash in _getTransactionReceipt? Beacause when i try to add bundleHash ,an error pops up about it not being included in providers.transactrionReceipt interface.

_getTransactionReceipt as of now:

    // TODO: bls-wallet #412 Update values returned in bundle receipt to more closely match ethers transaction response
    return {
      blockHash: bundleReceipt.blockHash,
      to: "0x",
      // bundleHash:bundleReceipt.bundleHash,
      from: "0x",
      contractAddress: "0x",
      transactionIndex: parseInt(bundleReceipt.transactionIndex),
      root:bundleReceipt.root,
      gasUsed: parseEther("0"),
      logsBloom: "",
      transactionHash: bundleReceipt.transactionHash,
      logs: [],
      blockNumber: parseInt(bundleReceipt.blockNumber),
      confirmations,
      cumulativeGasUsed: parseEther("0"),
      effectiveGasPrice: parseEther("0"),
      byzantium: false,
      type: 2,
    };
  }

JohnGuilding commented 1 year ago

@ri-dev-git you don't need to return the bundleHash in _getTransactionReceipt. The result from _getTransactionReceipt should fully match the ethers tx receipt interface

ri-dev-git commented 1 year ago

hi @JohnGuilding! I am trying to test the BlsProvider but i am stuck on an error:

i tried to dig into it but couldn't figure it out! Can you help me?

JohnGuilding commented 1 year ago

Hey @ri-dev-git, can you push up your changes to a branch in your fork, then post the link here? Then I can take a look

ri-dev-git commented 1 year ago

here it is: https://github.com/ri-dev-git/bls-wallet/tree/issue/

ri-dev-git commented 1 year ago

@JohnGuilding can you please review the return statement for lookupReceipt and eth_getTransactionReceipt:

lookupReceipt's return:

 return {
      transactionIndex: receipt.transactionIndex,
      transactionHash: receipt.transactionHash,
      bundleHash: hash,
      blockHash: receipt.blockHash,
      blockNumber: receipt.blockNumber,
      to: receipt.to,
      from: receipt.from,
      contractAddress: receipt.contractAddress,
      root: receipt.root,
      gasUsed: receipt.gasUsed,
      logsBloom: receipt.logsBloom,
      logs:receipt.logs,
      confirmations: receipt.confirmations,
      cumualtiveGasUsed: receipt.cumualtiveGasUsed,
      effectiveGasUsed: receipt.effectiveGasUsed,
      byzantium: receipt.byzantium,
      type: receipt.type,
    };

eth_getTransactionReceipt:

 bundleReceipt && {
          transactionHash: bundleReceipt.transactionHash,
          transactionIndex: bundleReceipt.transactionIndex,
          blockHash: bundleReceipt.blockHash,
          blockNumber: bundleReceipt.blockNumber,
          contractAddress: bundleReceipt.contractAddress,
          root: bundleReceipt.root,
          from: knownTx.from,
          to: knownTx.to,
          confirmations: bundleReceipt.confirmations,
          byzantium: bundleReceipt.byzantium,
          logs: [],
          type: bundleReceipt .type,
          cumulativeGasUsed: '0x0',
          gasUsed: '0x0',
          status: '0x1',
          effectiveGasPrice: '0x0',

JohnGuilding commented 1 year ago

@ri-dev-git looking into it today.

Can you confirm what version of these dependencies you've got: Node, Yarn, Deno, docker-compose?

Can you also confirm whether you've been able to get a local hardhat node running as well as a local version of the aggregator?

If not, follow the instructions here up until ./programs/aggregator.ts. After you've ran ./programs/aggregator.ts with no issues, you're in a good state to run the provider tests. (FYI the provider tests will not work correctly unless you have a local hardhat node and aggregator instance running.

The local hardhat node represents a local blockchain on your machine which is a very common web3 tool if you haven't come across it already. The aggregator is a bls-wallet abstraction in this case that submits transaction bundles to our smart contracts.

You'll need to make some changes to your aggregator ./env file if you haven't already. Any questions please shout.

Also, to make sure you're doing this from a clean slate in contracts, delete the following untracked folders in ./contracts if they exist:

./artifacts
./cache
./typechain-types

You can tell they're untracked because they'll be greyed out and won't show any changes in source control when you delete them.

ri-dev-git commented 1 year ago

Yes i will do the needed and get back at you!:)

ri-dev-git commented 1 year ago

node: using nvm i switched it to 16.0.0 yarn : 1.22.9 deno: 1.29.2 docker: 20.10.21

JohnGuilding commented 1 year ago

Ok perfect. Regarding versions, they look right 👍

for the return types:

BundleService.ts - lookupReceipt():

    return {
      bundleHash: hash,
      to: receipt.to,
      from: receipt.from,
      contractAddress: receipt.contractAddress,
      transactionIndex: receipt.transactionIndex,
      root: receipt.root,
      gasUsed: receipt.gasUsed,
      logsBloom: receipt.logsBloom,
      blockHash: receipt.blockHash,
      transactionHash: receipt.transactionHash,
      logs: receipt.logs,
      blockNumber: receipt.blockNumber,
      confirmations: receipt.confirmations,
      cumulativeGasUsed: receipt.cumulativeGasUsed,
      effectiveGasPrice: receipt.effectiveGasPrice,
      byzantium: receipt.byzantium,
      type: receipt.type,
      status: receipt.status,
      events: receipt.events
    };

Aggregator.ts - this is slightly different to how originally described:

export type BundleReceipt = ContractReceipt & {
  blsWallet: {
    bundleHash: string;
  };
};

This is an intersection type (denoted by the ampersand) and is more concise than writing all properties out, and will update easily if there are any updates to ethers. The reason we want the bundleHash within another object called blsWallet is because we want to clearly define a difference between the ethers receipt values and bls wallet values. One reason for this is that bls wallet bundles contain many operations that represent transactions e.g. transfer eth, interact with contract x etc. The values returned from the ethers receipt are related to the transaction to submit the entire bundle (which contains many operations). So values like the transaction hash will not relate to the operation the end user might be expecting with transaction.wait() when using the provider. See this page for a high level overview of bls-wallet architecture:

for eth_getTransactionReceipt, I'll have to get back to you on that one.

These return values and types might be subject to change at the PR stage. But if you use these for now, we'll have that discussion when you raise a PR.

ri-dev-git commented 1 year ago

Hi @JohnGuilding! I am able to run node in hardhat I have trouble with the command: yarn hardhat fundDeployer --network gethDev

I tried editing the mnemonic in .env file but it is popping out error as: The error shown after running the command:

And i get error while I try to run setup.ts file its an import error i dont knowif this is tsconfig error or deno.enable and also i couldn't find setting.json file for deno setting. please help as i dont know what track i am on!

error TS2691: An import path cannot end with a '.ts' extension. Consider importing 'https://deno.land/std@0.103.0/fs/mod' instead.

3 import { exists } from "https://deno.land/std@0.103.0/fs/mod.ts";

I am trying to resolve the problem but a hand could help! Thank you!

JohnGuilding commented 1 year ago

Hey @ri-dev-git

You don't need to edit the mnemonic in the contracts/.env file, you'll need to change that back to the default value.

Can you confirm what error you get after these steps:

Change mnemonic back to default value (see .env.example)
install this extension: https://marketplace.visualstudio.com/items?itemName=denoland.vscode-deno
run ./setup.ts. You just need to type the following into the terminal (no need for a deno prefix): ./setup.ts. Make sure you do this in the root directory.
cd into ./contracts, then run yarn hardhat node
in a separate terminal instance, cd into ./contracts and then run yarn hardhat fundDeployer --network gethDev

If you don't get anywhere with these steps, happy to jump onto a call tomorrow and get this resolved :) My timezone is CET

JohnGuilding commented 1 year ago

@ri-dev-git regarding deno setup, checkout https://deno.land/manual@v1.29.2/references/vscode_deno

By default, vscode comes with a built-in TypeScript/JavaScript language service. You'll need to change this by updating your User settings file and adding this line:

{
    ...other settings
    "deno.enablePaths": ["./aggregator", "./setup.ts"]
}

This will add support for Deno in those files/directories. You could also make these changes in your workspace settings - up to you

ri-dev-git commented 1 year ago

Sorry for late reply, iwas busy with some other work yes i will try it out and get back to you!

ri-dev-git commented 1 year ago

@JohnGuilding i followed the steps and got this in command line it seems that it has funded the deployer but has trouble to receive txnRes

JohnGuilding commented 1 year ago

@ri-dev-git interesting. I’d be happy to jump on a call to help solve this in about 2 hours if you were free?

When I tried using your repo the other day I was able to run that command successfully. It’s weird that it’s throwing an error.

In the meantime I would try and make sure you have a branch in a clean slate. Try deleting all untracked files (artifacts, cache, typecode types, .data etc)

Then I would go to your forked repo, pull latest from main and ensure your main is up to date with ours (there has been quite a few changes recently). Then create a new branch based off latest from main, and try following the setup steps from there (remember to update the aggregator env file with the local env settings).

ri-dev-git commented 1 year ago

anytime you free on 11th jan 11:00am CET? And sure i will fork the latest branch!

JohnGuilding commented 1 year ago

Sure I’m free then. If you send me your email I’ll book a zoom call.

ri-dev-git commented 1 year ago

ridev7875@gmail.com

JohnGuilding commented 1 year ago

@ri-dev-git I booked for 12pm CET but I can actually do 11:30 CET. If you can do that let me know and I'll update the meeting time :)

ri-dev-git commented 1 year ago

Works!

JohnGuilding commented 1 year ago

@ri-dev-git changed, you should be able to join now

ri-dev-git commented 1 year ago

hey @JohnGuilding ! i was successful in running all the commands up until docker-compose up -d postgres

But have trouble in running ./programs/aggregator.ts As said, i changed the config in env with RPC_URL and NETWORK_Config_PATH but no luck :(

JohnGuilding commented 1 year ago

Hey @ri-dev-git,

For a start, I would pull latest from main and update your aggregator .env file (see up-to-date properties in .env.local.example) as we have merged some changes here recently. Make sure your RPC_URL and NETWORK_CONFIG_PATH are still changed to the values given in local_development.md.

I would also check that your docker containers are running after docker-compose up -d postgres. You can do this easily by opening Docker Desktop and viewing the bls-wallet and postgres containers. Might not be relevant but see Docker WSL specific and DB WSL specific guides.

You mentioned you were running WSL, in which case there might be an issue related to how you access localhost. I haven't worked with WSL before. Worth looking into whether any of these answers correspond with your setup, and if they provide any insight (note these are related to accessing ports between the VM and Windows network which might not be your issue): https://superuser.com/questions/1131874/how-to-access-localhost-of-linux-subsystem-from-windows https://superuser.com/questions/1679757/how-to-access-windows-localhost-from-wsl2 https://superuser.com/questions/1594420/cant-access-127-0-0-180-outside-of-wsl2-ubuntu-20-04

Microsoft docs are generally quite helpful. It's worth looking into these if you haven't already. Here's a network specific page.

I'll also CC my teammate into this. @jacque006 we're having some issues getting @ri-dev-git set up with bls-wallet for local dev. Originally we couldn't even run the setup script but we figured out this was because he's running a windows machine. Correct me if I'm wrong @ri-dev-git but switching over to an ubuntu VM via WSL resolved this. Now we're having issues running ./programs/aggregator.ts. Wondering if you had any additional insight into the above.

JohnGuilding commented 1 year ago

Hey @ri-dev-git how's it going?

Do you need anymore support? :)

ri-dev-git commented 1 year ago

Hi @JohnGuilding! I have trouble starting up the docker with wsl2! When i tried to start docker desktop there was an error which was related to some directory permissions I tried digging into it and downloaded a docker image of postgres which led to some error with status of 500 in docker. I think I should either configure it in vm but i kinda have my exams and project to prepare for! If we can setup some final call and figure out a way one last time that would be helpful! Hope i wouldn't take much of your time!

JohnGuilding commented 1 year ago

Hey @ri-dev-git no worries!

I was going to suggest that I take over from here, as we could do with getting this work in over the next sprint.

Don't worry about it though, it's not your fault! It's on us to make sure that our repository can be run on Windows machines.

I think this conversation and some of the problems you've ran into are still really valuable to the project. So I'm going to raise a new issue to document the pain points when setting up a development environment on windows. This will be classed as a good first issue, if you're interested, I can assign you to it?

ri-dev-git commented 1 year ago

yes sure I would like to!

JohnGuilding commented 1 year ago

Ok here's the issue, feel free to assign yourself to it and get started! https://github.com/web3well/bls-wallet/issues/465

The issue is just to document the issues you ran into. Once we've done that, we'll think about implementation. Once we've got everything working, we've got plenty of good first issues you can choose from.

Any questions just shout :)

ri-dev-git commented 1 year ago

Yup!!

jacque006 commented 1 year ago

Resolved in https://github.com/getwax/bls-wallet/pull/480

getwax / bls-wallet

Update values returned in bundle receipt to more closely match ethers transaction response #435

Acceptance criteria