Fraschetti - Add tests for wallet_getWalletToken #19210
Conversation
Jenkins BuildsClick to see older builds (49)
|
| (defn assert-wallet-tokens | ||
| [tokens] | ||
| (let [flattened-tokens (mapcat val tokens)] | ||
| (is (true? (some #(= (:symbol %) "SNT") flattened-tokens))) |
There was a problem hiding this comment.
The true? call is unnecessary because some will return a boolean already.
FFFra
force-pushed
the
18342-wallet-add-integration-test-for-rpc-endpoint-wallet_getwallettoken
branch
from
bb95d7f to
f82da8d
Compare
FFFra
force-pushed
the
18342-wallet-add-integration-test-for-rpc-endpoint-wallet_getwallettoken
branch
2 times, most recently
from
1e174a8 to
36748c8
Compare
Thank you, I will take a look! |
FFFra
force-pushed
the
18342-wallet-add-integration-test-for-rpc-endpoint-wallet_getwallettoken
branch
from
36748c8 to
2a2cd01
Compare
| (defn assert-wallet-tokens | ||
| [tokens] | ||
| (let [flattened-tokens (mapcat val tokens)] | ||
| (is (some #(= (:symbol %) "SNT") flattened-tokens)) |
There was a problem hiding this comment.
I wonder is there anything else of importance to check in the tokens that we are dependent on.
i.e that there is some particular keys that we need
There was a problem hiding this comment.
Yah. I had the same feeling and now refactoring was also thinking about it. Maybe we just check if the tokens have been returned? Something like "response is not empty"
Opened to opinions
There was a problem hiding this comment.
oh no, I think being vague is less helpful. checking that symbol exists is a good thing, because that is part of the contract we are dependent on.
The best thing to do here would be too look at how we are using this rpc request and then check what parts of the data we need, then we know what we need to check in these contracts so that Status Go avoids making breaking changes (that will affect mobile) for this endpoint 👍
There was a problem hiding this comment.
@ilmotta any toughts?
There was a problem hiding this comment.
Nothing to add really to Jamie's comment above. I think that's a good description of the goal of all contract tests.
FFFra
force-pushed
the
18342-wallet-add-integration-test-for-rpc-endpoint-wallet_getwallettoken
branch
from
2a2cd01 to
b04605b
Compare
| main-account (get-main-account accounts) | ||
| response (contract-utils/call-rpc | ||
| "wallet_getWalletToken" | ||
| [main-account])] |
There was a problem hiding this comment.
The RPC call is now like "any" function call, so it accepts a variable number of args. In other words, we don't need to wrap the args in a vector.
There was a problem hiding this comment.
That's strange because for me it crashes if I remove it. But will investigate
FFFra
force-pushed
the
18342-wallet-add-integration-test-for-rpc-endpoint-wallet_getwallettoken
branch
5 times, most recently
from
0c3f6f2 to
265fd03
Compare
|
@J-Son89 can you please take another look? Made some changes. Thank you! |
FFFra
force-pushed
the
18342-wallet-add-integration-test-for-rpc-endpoint-wallet_getwallettoken
branch
from
265fd03 to
8f83a97
Compare
| _ (contract-utils/call-rpc | ||
| "accounts_saveAccount" | ||
| (data-store/<-account (merge default-account {:emoji test-emoji}))) | ||
| accounts (contract-utils/call-rpc "accounts_getAccounts")] | ||
| (check-emoji-is-updated test-emoji accounts))))) | ||
|
|
||
| (def number-of-networks 3) |
There was a problem hiding this comment.
imo this is better defined in the test file. Do really need this defined as a global constant? 🤔
There was a problem hiding this comment.
yah, I can roll back because this might change in future tests
| (defn assert-wallet-tokens | ||
| [tokens] | ||
| (let [flattened-tokens (mapcat val tokens)] | ||
| (is (some :symbol flattened-tokens)))) |
There was a problem hiding this comment.
I really think we should be testing more than this here.
Can you take a look through the code and see what properties of the tokens are being used and then ensure these are captured??
cc @ulisesmac, @smohamedjavid perhaps you could point to some uses of the data we are dependent on that we get from this rpc endpoint ("wallet_getWalletToken")
There was a problem hiding this comment.
Ideally, we should always receive the :symbol not being an empty string, but I remember we received some tokens named "" 😞
other important stuff is the Wei amount and :decimals, since we derive info from them
There was a problem hiding this comment.
The whole token data depends on it 😅.
Keys of a token that we use from wallet_getWalletToken RPC.
{:symbol "ETH"
:name "Ether"
:decimals 18
:balances-per-chain {1 {:raw-balance "193823235482934892"}
10 {:raw-balance "000004934800028760"}
...}
:market-values-per-currency {:usd {:price 3305.35 :change-pct-24hour -6.053133328320746}}}
;; NOTE: 1 and 10 are the chain ids of Ethereum mainnet and Optimism respectively. This would change if the user has enabled testnet.There was a problem hiding this comment.
Im pushing a new version. Do we need the :market-values-per-currency? Its the only one Im not testing
There was a problem hiding this comment.
@FFFra Yeah, we need it. we use it to calculate the fiat balance.
There was a problem hiding this comment.
Yes, we need to test it, as it is one of the essential keys used to calculate and display the fiat balance.
Just a check whether the keys market-values-per-currency and :usd (default currency) are present should suffice. (Because for some tokens (e.g. a bridged token), we receive 0 as value. Not needed to go deep)
FFFra
force-pushed
the
18342-wallet-add-integration-test-for-rpc-endpoint-wallet_getwallettoken
branch
from
793c3a5 to
c29fe89
Compare
src/tests/contract_test/utils.cljs
Outdated
| @@ -12,3 +12,11 @@ | |||
| :params args | |||
| :on-success p-resolve | |||
| :on-error p-reject})))) | |||
|
|
|||
| (defn get-main-account | |||
There was a problem hiding this comment.
Btw main account is not good to use because it could actually be the 'chat' account. You should just use that method use default account to guarantee it's related to wallet
| (let [raw-balance (:rawBalance balance)] | ||
| (is (not-empty raw-balance)) | ||
| (is (re-matches #"\d+" raw-balance)))))))) | ||
|
|
There was a problem hiding this comment.
Cc @smohamedjavid looks good here in the assertion? ^^
| (doseq [token flattened-tokens] | ||
| (is (not-empty (:symbol token))) | ||
| (is (:decimals token)) | ||
| (is (contains? token :balancesPerChain)) |
There was a problem hiding this comment.
A simple one up the top of my mind:
| (is (contains? token :balancesPerChain)) | |
| (is (contains? token :balancesPerChain)) | |
| (is (contains? token :marketValuesPerCurrency)) | |
| (is (contains? (:marketValuesPerCurrency token) :usd)) |
There was a problem hiding this comment.
why :marketValuesPerCurrency is important to the tests? (not against include it, just curious)
There was a problem hiding this comment.
No worries. It's just that we calculate the fiat balance in the client app (Mobile/Desktop) using the market values we get from status-go. Users love to see the fiat balance for the tokens they are holding. We don't want to disappoint the users 😄 .
There was a problem hiding this comment.
make sense. Thank you! I;'ve included
FFFra
force-pushed
the
18342-wallet-add-integration-test-for-rpc-endpoint-wallet_getwallettoken
branch
from
c29fe89 to
9bf0fca
Compare
|
Changes addressed! 🚀 |
FFFra
force-pushed
the
18342-wallet-add-integration-test-for-rpc-endpoint-wallet_getwallettoken
branch
from
9bf0fca to
4d19720
Compare
|
@smohamedjavid I Made some last refactors and will merge soon. Please let me know if there's something else. Thanks! |
FFFra
force-pushed
the
18342-wallet-add-integration-test-for-rpc-endpoint-wallet_getwallettoken
branch
from
4d19720 to
595acaa
Compare
FFFra
deleted the
18342-wallet-add-integration-test-for-rpc-endpoint-wallet_getwallettoken
branch
fixes #18342
follows #19208
Summary
This pull request introduces comprehensive tests for the
wallet_getWalletTokenendpoint, ensuring its robustness and reliability in our wallet functionality.Steps to test
run make test-contractsstatus: ready!