[#8026] [Wallet] Signing transaction #8361
Conversation
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (108)
|
flexsurfer
force-pushed
the
feature/tx-overview-screen
branch
from
50f8baf
to
7f0c6bd
Compare
| {:keys [value]} (wallet.db/parse-amount amount decimals) | ||
| amount' (str "0x" (abi-spec/number-to-hex (money/formatted->internal value symbol decimals))) | ||
| to (ethereum/public-key->address (:current-chat-id db)) | ||
| to' (ethereum/normalized-address (if (= symbol :ETH) to address)) |
There was a problem hiding this comment.
don't use these ' find a better name or thread the functions
flexsurfer
force-pushed
the
feature/tx-overview-screen
branch
2 times, most recently
from
b02f8de
to
adda0fb
Compare
flexsurfer
changed the title
src/status_im/browser/core.cljs
Outdated
| "NOTE (andrey) we need this function, because params may be mixed up, | ||
| so we need to figure out which one is address and which message" | ||
| [params] | ||
| (let [first_param (first params) |
There was a problem hiding this comment.
sorry for neatpicking, but why _ in names, but not -?
There was a problem hiding this comment.
this is something old, just moved here, but agree it's weird
src/status_im/browser/core.cljs
Outdated
| so we need to figure out which one is address and which message" | ||
| [params] | ||
| (let [first_param (first params) | ||
| second_param (second params) |
There was a problem hiding this comment.
you could use destructuring here:
[first-param second-param] params
There was a problem hiding this comment.
can you make this change since git doesn't understand that it is just moved anyway?
src/status_im/signing/core.cljs
Outdated
| (transaction-result cofx-in-progress-false result tx-obj)))) | ||
|
|
||
| (fx/defn discard | ||
| "Discrad transactrion signing" |
src/status_im/subs.cljs
Outdated
| amount-error (or (get-amount-error amount (:decimals token)) | ||
| (get-sufficient-funds-error balance (:symbol token) amount-bn))] | ||
| (if amount-error | ||
| amount-error |
There was a problem hiding this comment.
could be shorter with or
(or amount-error (get-sufficient-gas-error ...))
There was a problem hiding this comment.
based on @rasom comments during latest dev call, don't you think these are good candidate for local component state? while the functions themselves should stay separated from the UI and tested, does the whole app need to know about this state?
| @@ -62,7 +62,8 @@ | |||
| (if navigate-to-browser? | |||
| (fx/merge cofx | |||
| {:db (assoc-in db [:hardwallet :on-card-connected] nil)} | |||
| (wallet/discard-transaction) | |||
| ;;TODO use new signing flow | |||
| ;;(wallet/discard-transaction) | |||
There was a problem hiding this comment.
Could you change it to signing/discard as it seems like doing what wallet/discard-transaction did before
There was a problem hiding this comment.
Or maybe put it into comments for later
flexsurfer
force-pushed
the
feature/tx-overview-screen
branch
from
adda0fb
to
1e652ac
Compare
| amount-hex (str "0x" (abi-spec/number-to-hex (money/formatted->internal value symbol decimals))) | ||
| to (ethereum/public-key->address current-chat-id) | ||
| to-norm (ethereum/normalized-address (if (= symbol :ETH) to address)) | ||
| tx-obj (if (= symbol :ETH) |
There was a problem hiding this comment.
I am not a big fan of this tx-obj everywhere, it isn't really defined anywhere what it is about compared to just transaction
There was a problem hiding this comment.
in the end this tx-obj is just (select-keys transaction [:to :from :data])
There was a problem hiding this comment.
i've made tx-obj separate because in that case I'm sure it won't be changed and will be signed as is, if we mix it with a transaction for signing screen, it might be messed
| :data (when (and method params) (abi-spec/encode method params))) | ||
| transaction (prepare-extension-transaction tx-object (:contacts/contacts db) on-success on-failure)] | ||
| (wallet/open-modal-wallet-for-transaction db transaction tx-object))) | ||
| (signing/sign {:db db} {:tx-obj (assoc (select-keys arguments [:to :gas :gas-price :value :nonce]) |
There was a problem hiding this comment.
why not just transaction? so far we have been pretty strict about not using abbreviations in the code base and I don't see how it is an object
There was a problem hiding this comment.
it's object from here https://github.com/ethereum/wiki/wiki/JavaScript-API#parameters-25, Object - The transaction object to send:
but i'm open for suggestions on better naming
| (:require [status-im.utils.fx :as fx] | ||
| [status-im.i18n :as i18n] | ||
| [status-im.utils.money :as money] | ||
| [status-im.utils.hex :as utils.hex] |
There was a problem hiding this comment.
can you run import clean (remove unused/order alphabetically)
|
@flexsurfer When signing message from Dapp (from CryptoKitties or from test dapp) there is overlap with Title in the app and status bar of the phone (Galaxy S6, Android 6.0.1 and Pixel XL, Android 9) note that it doesn't happen on Galaxy S10, Pixel 3 and iphone 7 plus
|
84% of end-end tests have passedFailed tests (8)Click to expand
Passed tests (41)Click to expand |
|
@flexsurfer please check test_send_two_transactions_one_after_another_in_dapp |
yes it is not working anymore, screen is not responsive after first txn is signed. I bet it was working when i was testing it. |
96% of end-end tests have passedFailed tests (2)Click to expand
Passed tests (45)Click to expand
|
flexsurfer
force-pushed
the
feature/tx-overview-screen
branch
from
17ad9c4
to
d1b991f
Compare
|
@asemiankevich fixed |
96% of end-end tests have passedFailed tests (2)Click to expand
Passed tests (45)Click to expand
|
|
@antdanchenko the problem is fixed as i checked manually but test is failed because it does not tap on button second time. Could you please take a look? https://ethstatus.testrail.net/index.php?/tests/view/715952 |
50% of end-end tests have passedFailed tests (1)Click to expand
Passed tests (1) |
|
All e2e should be fixed now. Single test above failed due to faucet didn't work, but it's not this PR issue (manually all fine) |
|
@annadanchenko would you mind if this branch will be merged and the issue u are referring to will be fixed in develop separately? |
Signed-off-by: Andrey Shovkoplyas <motor4ik@gmail.com>
flexsurfer
force-pushed
the
feature/tx-overview-screen
branch
from
b72fd2b
to
f3aa376
Compare
fixes #8026
The new method is introduced for signing transactions
signing/signhttps://github.com/status-im/status-react/pull/8361/files#diff-598cee30285d1d51df0def4398019d98R234
Usage
FOR QA and Design teams:
this PR doesn't include:
FOR QA:
we had default gas 105000 for tokens, in current implementation we rely on gas estimation by the network, but sometimes it fails and we'll show error tooltip for gas in that case, also we need to verify that gas estimation works properly
TESTED:
NOT TESTED:
-sending real eth or snt
CONCERNS: