Added radio button toggle to use Web3 provider to send transaction #146

VSaric · 2021-12-23T15:06:09Z

fixes: #137

Description: Added a radio button toggle on the top of the page. The options in the toggle is ethers and web3. Default toggle is ethers. If ethers is selected, the create token and deploy contract buttons are behave as they do today. If web3 is selected, create token and deploy contract use web3 instead of the current factories created by the ethers provider. The transactions created by the buttons are the same.

Screenshot of radio button toggle:

Switching ethers/web3 sreencast:

Screencast.2021-12-27.11.23.37.mp4

david0xd · 2021-12-24T09:42:08Z

src/index.html

@@ -30,6 +30,16 @@ <h1 id="logo-text" class="text-center">
        </div>
      </header>
      <section>
+        <form name="myForm" class="form">


This name can be something like "librarySwitchForm".

Fixed in: 91555c0

david0xd · 2021-12-24T09:45:24Z

src/index.css

+  margin: 0 5px 0 5px;
+}
+
+#libraryLabel {


This should be a class instead of an ID selector -> .libraryLabel.

Fixed in: 91555c0

david0xd · 2021-12-24T09:45:54Z

src/index.html

+          <div class="row justify-content-center">
+            <div class="justify-content-center">
+              <input type="radio" id="radio-ethers" name="libraryUsed" value="ethers" />
+              <label id="libraryLabel">Ethers</label>


Replace id with class here for both labels.

Fixed in: 91555c0

david0xd · 2021-12-24T09:49:56Z

src/index.js

+function handleLibraryChange(event) {
+  libraryInUse = event.target.value;
+  if (libraryInUse === 'web3') {
+    // Deployed piggy bank smart contract with web3 library


We can remove these comments for this particular section because code already looks self-explainable.

Fixed in: 91555c0

david0xd · 2021-12-24T09:50:08Z

src/index.js

+    // Deployed failing smart contract with web3 library
+    failingContract = web3FailingContract;
+  } else {
+    // Deployed piggy bank smart contract with ethers library


We can remove these comments for this particular section because code already looks self-explainable.

Fixed in: 91555c0

david0xd · 2021-12-24T09:53:12Z

src/index.js

+    depositButton.disabled = false;
+    withdrawButton.disabled = false;
+  } else {
+    contractStatus.innerHTML = 'Not clicked';


This should probably say something like "Not deployed" instead of "Not clicked".

Fixed in: 91555c0

david0xd · 2021-12-24T09:54:22Z

src/index.js

+    failingContractStatus.innerHTML = 'Deployed';
+    sendFailingButton.disabled = false;
+  } else {
+    failingContractStatus.innerHTML = 'Not clicked';


This should probably say something like "Not deployed" instead of "Not clicked".

Fixed in: 91555c0

david0xd · 2021-12-24T09:54:43Z

src/index.js

+    sendFailingButton.disabled = true;
+  }
+
+  console.log(libraryInUse);


This console log can be removed.

Fixed in: 91555c0

david0xd · 2021-12-24T09:59:54Z

src/index.js

+            .then((receipt) => {
+              console.log(receipt);
+            });
+          contractStatus.innerHTML = 'Deposit completed';


This should be moved in then() callback function instead of being here.
Also, it would be nice to have catch on error and handle that as well (log to console and display status text with error message).

Fixed in: 91555c0

david0xd · 2021-12-24T10:01:04Z

src/index.js

+            .then((receipt) => {
+              console.log(receipt);
+            });
+          contractStatus.innerHTML = 'Withdrawn';


This can also be moved to then() and error catch is welcome.

Fixed in: 91555c0

david0xd · 2021-12-24T10:01:17Z

src/index.js


-      console.log(contract);
+      console.log(piggyBankContract);


This can be removed.

Fixed in: 91555c0

mirjanaKukic · 2021-12-28T11:01:51Z

Verified by QA.

VSaric · 2022-01-13T13:21:04Z

Hello @danjm, we implemented the web3 switching feature into the test-dapp and you can see it here in this PR. Just to remember you, if you have time, can you please review this changes, because when this PR be merged and done, we can unblocked ticket #18 and move it from Pending Consensys to Ready for development column.

Thank you in advance! :)

EHaracic · 2022-01-21T10:24:40Z

fixes: #137

mcmire

I tested these changes, and they seem to work.

There's one thing about this PR that we need to think about. This will add some tech debt that I think we will need to resolve in order to be able to make it easy to maintain the test dapp in the future. First, after this PR, the size of src/index.js will be 1668 lines. It's not the most pleasant experience to maintain a file like this, and I think it'd be good if we split this up into separate files. Second, since we now have two ways of doing the same thing, I think we could benefit from using the strategy pattern to represent the logic between Web3 and ethers. That is, we could figure out all of the common actions between the two strategies and then create a Web3 object and an ethers object that implements the differences among all of the actions we've defined, then switch out the objects when the radio button is clicked.

I don't know if this change is time-sensitive, so if that's the case, I'm fine if we merge this now and refactor this later, but we're getting to a point now where the more we change, the more debt we create and the longer it will take to refactor. So it might be something we want to think about sooner rather than later.

david0xd · 2022-01-31T10:45:05Z

@mcmire I agree with you, just the purpose of this was to unblock the #18 at the moment.
Even before web3 integration was started, that index.js requires some heavy refactoring.
So, we should create a new task to do some good refactoring, probably to split these blocks of functionalities into the separate files, do the switching and call-delegation at the higher level from the index.js and propagate forward.

@danjm can you consider creating new ticket for refactoring of the test-dapp (in complete)? I'm also wondering if this is a part of the TypeScript refactoring and should this also be TypeScript so we do all at once?

I would just suggest that we finish with this with the merge and implementation of the batch transactions to unblock the requirements and then do the refactor.

EHaracic · 2022-02-04T09:32:03Z

@danjm can we merge this issue?

danjm · 2022-02-10T11:22:39Z

I checked that this is compatible with extension e2e tests

danjm · 2022-02-10T11:23:46Z

I think we are close to being able to merge this. Before we do I'd like to get a look at this from @PeterYinusa: Peter, does this meet the needs we have for avoiding web3 compatibility bugs, from a QA perspective?

PeterYinusa · 2022-04-14T15:40:44Z

I think we are close to being able to merge this. Before we do I'd like to get a look at this from @PeterYinusa: Peter, does this meet the needs we have for avoiding web3 compatibility bugs, from a QA perspective?

Yes, this looks good

mcmire

LGTM.

…currently used

…s to use MM estimation

mcmire

This all looks good to me still!

mcmire · 2022-12-14T00:20:54Z

What was the final status on this PR, did we decide we didn't need it any more?

danjm · 2022-12-14T12:25:12Z

The changes from this PR are going to be incorporated into future changes to the test dapp made by the dapp api / devrel team

VSaric self-assigned this Dec 23, 2021

david0xd reviewed Dec 24, 2021

View reviewed changes

VSaric marked this pull request as ready for review December 28, 2021 11:09

VSaric requested a review from a team as a code owner December 28, 2021 11:09

VSaric requested a review from david0xd December 28, 2021 11:09

david0xd previously approved these changes Dec 28, 2021

View reviewed changes

VSaric dismissed david0xd’s stale review via f5f97d9 January 21, 2022 15:05

mcmire previously approved these changes Jan 24, 2022

View reviewed changes

david0xd mentioned this pull request Jan 31, 2022

Implement batch transactions #18

Open

david0xd previously approved these changes Jan 31, 2022

View reviewed changes

VSaric dismissed stale reviews from david0xd and mcmire via 7f246d8 February 4, 2022 10:14

VSaric requested review from david0xd, mcmire and danjm February 4, 2022 10:15

mcmire previously approved these changes Feb 4, 2022

View reviewed changes

VSaric dismissed mcmire’s stale review via fe289d5 February 7, 2022 11:18

VSaric requested a review from mcmire February 7, 2022 11:20

david0xd previously approved these changes Feb 8, 2022

View reviewed changes

VSaric dismissed david0xd’s stale review via c2d384c April 12, 2022 13:44

mcmire previously approved these changes May 5, 2022

View reviewed changes

VSaric added 5 commits June 17, 2022 11:36

Allowing the test dapp to use web3 in place of ethers wherever it is …

98e760c

…currently used

Added web3 library to work with failing contracts

25b5184

Added necessary scripts into package.json and changed deploy function…

1d22c1b

…s to use MM estimation

Review comments update

8f89a19

Added missing libraries in yarn.lock file after rebase

9dce0d9

VSaric dismissed mcmire’s stale review via 9dce0d9 June 17, 2022 09:38

VSaric requested a review from mcmire June 20, 2022 08:10

mcmire approved these changes Jun 21, 2022

View reviewed changes

david0xd closed this by deleting the head repository Dec 8, 2022

Added radio button toggle to use Web3 provider to send transaction #146

Added radio button toggle to use Web3 provider to send transaction #146

Conversation

VSaric commented Dec 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirjanaKukic commented Dec 28, 2021

VSaric commented Jan 13, 2022

EHaracic commented Jan 21, 2022

mcmire left a comment

Choose a reason for hiding this comment

david0xd commented Jan 31, 2022

EHaracic commented Feb 4, 2022

danjm commented Feb 10, 2022

danjm commented Feb 10, 2022

PeterYinusa commented Apr 14, 2022

mcmire left a comment

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

mcmire commented Dec 14, 2022

danjm commented Dec 14, 2022

VSaric commented Dec 23, 2021 •

edited

Loading