-
Notifications
You must be signed in to change notification settings - Fork 985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add request address command #9741
Conversation
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (64)
|
e8ccee7
to
60a4698
Compare
@rachelhamlin requesting a transaction has also been implemented if you'd like to give it a go |
60a4698
to
392c291
Compare
✔️ status-react/prs/ios/PR-9741#6 🔹 ~12 min 🔹 9adce9e 🔹 📦 ios package |
✔️ status-react/prs/ios/PR-9741#7 🔹 ~9 min 10 sec 🔹 4cc252a 🔹 📦 ios package |
UX bug turned out to be quite a rabbit hole, while implementing link to transaction I realize that the lack of pending transaction in history was actually due to a bug, which I fixed, then while testing it it turned out that there was also a bug in send transaction drawer when trying to select one of your own account as recipient. |
96% of end-end tests have passed
Failed tests (4)Click to expand
Passed tests (96)Click to expand |
With the 14th builds in jenkins on both iOS and Android for the accounts switched to Ropsten network and created 1-1 chat between each other: Issue #1Similar to above Issue1: Issue #2Can't Cancel bottom-sheet which appears after |
Issue 3There are issues with keyboards appearance when user taps
Scenario #1 (When dialpad on Android appears)
Scenario #2 (When keyboard on Android appears)
Scenario #3 (When keyboard on iOS keep staying overlapping Send tranasaction bottom-sheet)
Expected results:I think expect results for above would be: when |
@Serhy build 17 should contain fixes for all your issues, also I switched to ropsten instead of mainnet so you can do all the tests you want. build 16 is the same but mainnet only |
src/status_im/node/core.cljs
Outdated
@@ -134,6 +134,8 @@ | |||
:InstallationID installation-id | |||
:MaxMessageDeliveryAttempts config/max-message-delivery-attempts | |||
:MailServerConfirmations config/mailserver-confirmations-enabled? | |||
:VerifyTransactionURL "https://ropsten.infura.io/v3/f315575765b14720b32382a61a89341a" #_"https://mainnet.infura.io/v3/f315575765b14720b32382a61a89341a" | |||
:VerifyTransactionChainID 3 #_1 |
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.
whats this?
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.
we use that to validate transaction, it should always point to mainnet as we don't validate ropsten transactions, so it's not the same as the network the user is in
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.
though once we support multiple networks it can probably go away
[cofx chat-id transaction-hash signature] | ||
{::json-rpc/call [{:method "shhext_sendTransaction" | ||
:params [chat-id transaction-hash (:result (types/json->clj signature))] | ||
:on-success #(re-frame/dispatch [:transport/message-sent % 1])}]}) |
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.
what does 1 mean ?
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.
1 means that only 1 confirmation is needed (as opposed to n
for group chats), it's an artifact of when sending was mostly in status-react, we will remove it at some point but still some work to do
@@ -37,7 +37,7 @@ | |||
:height panel-height | |||
:transform [{:translateY bottom-anim-value}] | |||
:opacity alpha-value}} | |||
[react/view | |||
[react/view {:style {:flex-direction :row}} |
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.
why row ? @errorists
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.
@errorists can tell if that's a good idea or not, basically I kept finding it super weird to have only 2 commands that are aligned vertically instead of horizontally, in the designs they are on top of each other but there is also more commands. So I thought we should align them like this until there is more
|
||
(defn- command-pending-status | ||
[command-state direction to transaction-type] | ||
[react/view {:style {:flex-direction :row |
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.
could be moved to styles ns
✔️ status-react/prs/ios/PR-9741#19 🔹 ~9 min 9 sec 🔹 090e810 🔹 📦 ios package |
Issue #4Status crashes when navigating to 1-1 chat with sent or received To reproduce:
Logcat with exception stacktrace: |
ISSUE 5: Messages from device with custom time are not sent immediatelyI think it may be important because issue is not reproducible on current nightly. Steps:
Expected result: message is received on Device B |
@churik can you replicate consistently? are messages on device A marked as "Sent"? |
@cammellos, yes, consistently from PR build. |
25d2fa0
to
ebc7051
Compare
All above issues are fixed but Issue 3: Scenario1 and Scenario2 on Android 8.1 (Samsung J7) (can't reproduce with Android 9 though) Issue 6Custom token displayed as Update: in latest build here difference between develop for #9792 issue is that here custom token kept on To reproduce:
Issue 7We need to reflect testnet ETH as we do in Wallet: i.e. ETHro/ETHri. It's now displayed as Issue 8Commands view in different network. Not sure if it's part of this PR but what happens is that commands I sent from Ropsten looks on Mainnet as |
c4e8012
to
fddd865
Compare
Tested https://i.diawi.com/3KL5uB,
Lets merge, @cammellos @yenda (just to fix conflict). Kudos to you! |
Signed-off-by: yenda <eric@status.im>
This PR adds the request-address command as described in the specs.
The flow is:
Currently the validation is done on Ropsten for testing purposes, so make sure both accounts are using the ropsten network before testing.
Known issues:
Likely there are other issues here and there, if testing, please let me know.
status: ready