Feature for confirm custom transaction #306

Tarzeron · 2019-08-30T14:13:01Z

No description provided.

change style of JSON tx

mvines

Neat. It would be helpful to add more detail into the README about what the purpose of "confirmTransaction" is. Also it looks like it's really "sendCustomTransaction", as the functionality first sends the transaction and then confirms it.

mvines · 2019-08-30T20:25:06Z

README.md

@@ -55,7 +55,7 @@ window.addEventListener('message', (e) => {
  }
 });
 ```
-4. Send an `'addFunds'` request
+4. Send an `'addFunds'` or `'confirmTX'` request


confirmTransaction please.

mvines · 2019-08-30T20:25:17Z

README.md

+walletWindow.postMessage({
+  method: 'confirmTX',
+  params: {
+    description: "Description of tx",


tx -> transaction

mvines · 2019-08-30T20:26:08Z

README.md

+  params: {
+    description: "Description of tx",
+    format: 'JSON',
+    transaction: "Your tx in JSON",


Would be nice to describe this more. Is this a JSON array of octets representing the signed wire transaction, or some other kind of JSON representation

mvines · 2019-08-30T20:27:27Z

src/wallet.js

@@ -151,14 +153,15 @@ class TokenInput extends React.Component {
              placeholder="Enter amount to transfer"
              onChange={e => this.handleChange(e.target.value)}
            />
-            <FormControl.Feedback />
+            <FormControl.Feedback/>


Can you please remove all these whitespace changes? Ie, formatting style changes can be a separate PR, much easier to review the "important" bits of this PR

…llet into develop # Conflicts: # src/wallet.js

mvines · 2019-09-10T15:57:41Z

@jstarry - hey can you please review this PR?

jstarry

Great! Thanks for adding this functionality, super useful :D

jstarry · 2019-09-10T17:13:14Z

src/wallet.js

+    const transaction = new web3.Transaction();
+    const inputs = JSON.parse(params.transaction);
+
+    inputs.map(input  => {


I'm not a big fan of this approach because if we change Transaction structure, it would break. I would prefer to rely on the web3 sdk serialization methods instead for passing messages: transaction.serialize(): Buffer and Transaction.from(buffer: Buffer).

I like that you are displaying the json formatted transaction in the confirmation before sending. I think that a toJSON method would be nice to have inside Web3, thoughts?

Yes, I also wanted to do this, but SDK does not serialize an unsigned transaction, can you give an example how to do it?

public serialize(): Buffersource Serialize the Transaction in the wire format. The Transaction must have a valid signature before invoking this method I have error Uncaught Error: Transaction recentBlockhash required

Check out solana-labs/solana-web3.js@85b354e

need to fill out recentBlockhash, what if the user did not immediately confirm the transaction, it needs to be filled again on the wallet side?

Setting a placeholder is fine because the wallet will always set the recentBlockhash when signing. Something like:

transaction.recentBlockhash = new PublicKey(0).toBase58();

jstarry · 2019-09-10T17:14:36Z

src/wallet.js

+  onSendCustomTransactionRequest(params, origin) {
+    if (!params.format || !params.network || !params.transaction) {
+      if (!params.network) this.addError(`Request did not specify a network`);
+      if (!params.format) this.addError(`Request did not specify a transaction format`);


Looks like you aren't checking the value of format anywhere. Let's remove it for now, and add if we introduce other formats.

jstarry · 2019-09-10T17:24:37Z

README.md

@@ -55,7 +55,7 @@ window.addEventListener('message', (e) => {
  }
 });
 ```
-4. Send an `'addFunds'` request
+4. Send an `'addFunds'` or `'sendCustomTransaction'` request


We need to re-organize this README section rather than adding more things to the "Funding dApps" section. I recommend

## Wallet Setup ... steps 1 - 3 ## Requesting Funds ... steps 4 - 5 ## Sending Custom Transactions ... your new steps

jstarry · 2019-09-10T17:25:31Z

src/wallet.js

@@ -321,14 +321,16 @@ export class Wallet extends React.Component {
    account: null,
    requestMode: false,
    requesterOrigin: '*',
-    requestPending: false,


Why did you remove this?

The user may not close the wallet window after the last request and may simply not respond to it, but may also request a new action in the DAPP therefore. For useful, I allowed DAPP to update the request, and in the future we will need a request queue.

jstarry · 2019-09-10T17:27:00Z

src/wallet.js

    requestedPublicKey: '',
    requestedAmount: '',
    recipientPublicKey: '',
    recipientAmount: '',
    recipientIdentity: null,
    confirmationSignature: null,
    transactionConfirmed: null,
+    unsignedTransaction: null,
+    formattedUnsignedTransaction: '',
+    description: '',


Please namespace this: unsignedTransactionDescription is more wordy but easier to understand what it's used for

jstarry · 2019-09-10T17:31:03Z

src/wallet.js

@@ -321,14 +321,16 @@ export class Wallet extends React.Component {
    account: null,
    requestMode: false,


Let's change this state flag from a boolean to an enum and rename to displayMode since we have 3 display modes now

jstarry · 2019-09-10T17:32:40Z

src/wallet.js

+      description: params.description ? params.description : '',
+      unsignedTransaction: transaction,
+      formattedUnsignedTransaction: JSON.stringify(JSON.parse(params.transaction), null, 4),
+      requestedPublicKey: null,


please remove, we can use a different approach to distinguish between "addFunds" and "sendCustomTransaction"

src/wallet.js

jstarry · 2019-09-10T18:19:08Z

src/wallet.js

@@ -817,10 +906,46 @@ export class Wallet extends React.Component {
            </Col>
          </Row>
        </Grid>
+        <div className="container">{this.renderPanels()}</div>


Please revert this change

Tarzeron2 and others added 11 commits August 20, 2019 10:51

ignore IDE files

3632946

fix convert tx from JSON

9d87cd3

remove debug code

c448d8b

Merge branch 'feature/request_confirm_tx' into develop

f4abf5c

add description for confirm tx request

48e86ff

change style of JSON tx

if have request on confirm we clear transfer request

14a2581

allow replace request in window

1dae483

fix condition for disable send button

91c83ca

fix transaction serialize if tx have many instruction

f1b43b4

move send button up

e2bf87b

update read.me

464d6cf

mvines reviewed Aug 30, 2019

View reviewed changes

newcodepusher added 3 commits September 10, 2019 14:02

revert style for pr

b77cd48

update read.me

49bf426

rename request

acd1538

Tarzeron force-pushed the develop branch from cde6144 to acd1538 Compare September 10, 2019 12:02

newcodepusher added 2 commits September 10, 2019 15:43

rename request

cf9df05

Merge branch 'master' of https://github.com/solana-labs/example-webwa…

4c741aa

…llet into develop # Conflicts: # src/wallet.js

mvines requested a review from jstarry September 10, 2019 15:57

jstarry suggested changes Sep 10, 2019

View reviewed changes

newcodepusher added 6 commits September 11, 2019 10:05

refactor, rename description of unsigned transaction

4a66a1c

fix merge mistake

e75f537

remove format, required field for custom transaction

ab5bc9b

re-organize README

15c86ed

revert confirmTransaction

217d344

add display mode for determine request type

a528678

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature for confirm custom transaction #306

Feature for confirm custom transaction #306

Tarzeron commented Aug 30, 2019

mvines left a comment

mvines Aug 30, 2019

Tarzeron Sep 10, 2019

mvines Aug 30, 2019

Tarzeron Sep 10, 2019

mvines Aug 30, 2019

Tarzeron Sep 10, 2019

mvines Aug 30, 2019

Tarzeron Sep 10, 2019

mvines commented Sep 10, 2019

jstarry left a comment

jstarry Sep 10, 2019

Tarzeron Sep 11, 2019 •

edited

Loading

mvines Sep 11, 2019

Tarzeron Sep 11, 2019 •

edited

Loading

jstarry Sep 11, 2019

jstarry Sep 10, 2019

Tarzeron Sep 11, 2019

jstarry Sep 10, 2019

Tarzeron Sep 11, 2019

jstarry Sep 10, 2019

Tarzeron Sep 11, 2019

jstarry Sep 10, 2019

Tarzeron Sep 11, 2019

jstarry Sep 10, 2019

Tarzeron Sep 11, 2019

jstarry Sep 10, 2019

Tarzeron Sep 11, 2019

jstarry Sep 10, 2019

Tarzeron Sep 11, 2019

		@@ -321,14 +321,16 @@ export class Wallet extends React.Component {
		account: null,
		requestMode: false,

Feature for confirm custom transaction #306

Are you sure you want to change the base?

Feature for confirm custom transaction #306

Conversation

Tarzeron commented Aug 30, 2019

mvines left a 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

mvines commented Sep 10, 2019

jstarry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tarzeron Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tarzeron Sep 11, 2019 • 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

Tarzeron Sep 11, 2019 •

edited

Loading

Tarzeron Sep 11, 2019 •

edited

Loading