feat(jumpstart): handle jumpstart state #4963

bakoushin · 2024-02-23T10:57:57Z

Description

Introducing new slice to the redux state for jumpstart link handling:

jumpstart: {
  showLoading: boolean
  showError: boolean
}

This state is expected to be used for rendering UI components displaying the jumpstart link handling state to the user. The actual components will follow in the subsequent PR once the design discussions converge.

After successful jumpstart claim two types of pending transactions are dispatched (getting tx data trough blockchain RPC calls):

Payment received (for ERC-20 claim)
NFT received (for ERC-721 claim)

Those transactions are expected to be replaced with blockchain-api data once it's fetched.

Added analytics events for jumpstart handler lifecycle:

claim succeeded
claim failed
ERC20 token claimed
ERC721 nft claimed

Test plan

Updated unit tests

Related issues

Related to RET-1004

Backwards compatibility

Y

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

codecov · 2024-02-23T11:28:45Z

Codecov Report

Attention: Patch coverage is 98.50746% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 85.48%. Comparing base (b2b7890) to head (86e4e85).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4963      +/-   ##
==========================================
+ Coverage   85.43%   85.48%   +0.05%     
==========================================
  Files         721      724       +3     
  Lines       29435    29534      +99     
  Branches     5093     5100       +7     
==========================================
+ Hits        25148    25248     +100     
+ Misses       4048     4047       -1     
  Partials      239      239

Files	Coverage Δ
src/analytics/Events.tsx	`100.00% <100.00%> (ø)`
src/analytics/Properties.tsx	`100.00% <ø> (ø)`
src/app/saga.ts	`73.16% <100.00%> (+0.09%)`	⬆️
src/jumpstart/saga.ts	`100.00% <100.00%> (ø)`
src/jumpstart/selectors.ts	`100.00% <100.00%> (ø)`
src/jumpstart/slice.ts	`100.00% <100.00%> (ø)`
src/nfts/types.ts	`100.00% <ø> (ø)`
src/redux/migrations.ts	`97.27% <100.00%> (+<0.01%)`	⬆️
src/redux/reducers.ts	`94.59% <100.00%> (+0.15%)`	⬆️
src/redux/store.ts	`79.36% <ø> (ø)`
... and 4 more

... and 2 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2b7890...86e4e85. Read the comment docs.

kathaypacific

looking great!! perhaps you want to add the jumpstart state to the redux blacklist or restore the initial state via the rehydrate action, but it seems like we wouldn't want to persist this store across app sessions?

kathaypacific · 2024-02-26T10:37:27Z

src/jumpstart/saga.ts

+    yield* put(jumpstartClaimStarted())
+
+    const transactionHashes = yield* call(jumpstartLinkHandler, privateKey, walletAddress)
+    yield* fork(dispatchPendingTransactions, transactionHashes)


i think we don't do a great job of handling multiple chains here. on one hand, we've made the dynamic config work for multichain by enabling a different contract address for each chain but in the code we're only looking for the defaultNetworkId. in both the jumpstartLinkHandler and 'dispatchPendingTransactions' sagas only look at the default network.

could we update this to be something like get all the supported networks based on what is there in the dynamic config -> for each chain, fork a saga that calls jumpstartLinkHandler with the chain id and then does the dispatchPendingTransactions for those transactions?

I agree that we should handle networks in a generic way and avoid network hardcoding. Thanks for pointing this out!

I ended up looping through all remote config networkIds, but I think it's suboptimal. If the amount of jumpstart-supported networks grows, we would have to loop trough them all, issuing network/blockchain request for every network because we have no idea for which specific network that link is. To avoid looping, perhaps we can encode network id within the jumpstart link?

Also, we probably want to refactor jumpstartLinkHandler from ContractKit to Viem to make it multichain?

For now, I just made it throw an error if network id isn't Celo.

kathaypacific · 2024-02-26T10:55:15Z

src/jumpstart/jumpstartLinkHandler.ts

+  const transactionHashes = [
+    ...(await executeClaims(kit, jumpstart, publicKey, userAddress, 'erc20', privateKey)),
+    ...(await executeClaims(kit, jumpstart, publicKey, userAddress, 'erc721', privateKey)),
+  ]


nit: could we do a Promise.all here?

kathaypacific · 2024-02-26T10:58:58Z

src/analytics/Properties.tsx

+interface WalletJumpstartProperties {
+  [JumpstartEvents.jumpstart_started]: undefined
+  [JumpstartEvents.jumpstart_succeeded]: undefined
+  [JumpstartEvents.jumpstart_failed]: undefined


i wonder if we need the jumpstart_started event, when it will always end with jumpstart_succeeded or jumpstart_failed? maybe i'm missing the extra information we get from this event.

i also wonder if it'd be nice to capture some data on how users are using this feature, e.g. the token and amounts that are being claimed. i guess we can also see this on the contract events directly, but maybe it helps with data analysis to capture this in analytics? i'm not sure, just a suggestion :)

yes, jumpstart_started is redundant.

added separate events for token/amount tracking:

jumpstart_claimed_token with ERC20 claim details

jumpstart_claimed_nft with ERC721 claim details

I think it's worth keeping a bare jumpstart_succeeded event as well, because getting claim details may fail (but it doesn't mean claim didn't succeeded)

emerge-tools · 2024-02-26T18:39:17Z

1 build increased size

Name	Version	Download	Change	Install	Change	Approval
Celo (test) org.celo.mobile.test	1.79.0 (144)	24.2 MB	⬆️ 4.3 kB (0.02%)	60.3 MB	⬆️ 12.3 kB (0.02%)	N/A

Celo (test) 1.79.0 (144)
org.celo.mobile.test

⚖️ Compare build
📦 Install build
⏱️ Analyze build performance

Total install size change: ⬆️ 12.3 kB (0.02%)
Total download size change: ⬆️ 4.3 kB (0.02%)

Largest size changes

Item	Install Size Change
main.jsbundle	⬆️ 12.3 kB

🛸 Powered by Emerge Tools

bakoushin · 2024-02-27T10:07:34Z

perhaps you want to add the jumpstart state to the redux blacklist or restore the initial state via the rehydrate action, but it seems like we wouldn't want to persist this store across app sessions?

yes, this state shouldn't persist, because it's only for presentation purposes during current user session. added it to persist's blacklist. thanks for pointing this out!

kathaypacific

looking great!! sorry i had a different thought while reviewing how we make the claim here, lmk what you think

kathaypacific · 2024-02-27T13:07:28Z

src/jumpstart/jumpstartLinkHandler.ts

+  userAddress: string
+): Promise<Hash[]> {
+  if (networkId !== networkConfig.defaultNetworkId) {
+    throw new Error(`Unsupported network id: ${networkId}`)


ah, i was wondering why we need to do this, and then i saw that this function is using contract kit 😞 let's leave it for now and make a task to remove contract kit for later

ok, here is the task: https://linear.app/valora/issue/RET-1019/[wallet]-refactor-jumstartlinkhandler-to-viem

kathaypacific · 2024-02-27T13:15:53Z

src/jumpstart/saga.ts

+
+        yield* fork(dispatchPendingTransactions, networkId, transactionHashes)
+      } catch (error) {
+        Logger.error(TAG, `Error handling jumpstart link for ${networkId}`, error)


this claim saga is kicked off every time a jumpstart deep link is consumed, but presumably each deep link is only for one network. if we had multichain jumpstart, this logger.error would always be fired because some chains will always have no transactions found right? and we'd always be doing unnecessary work, to try to claim across chains when we should only do it for one chain? perhaps what would be more efficient is to have the jumpstart link contain the network id...what do you think? sorry, i know this is different to what i suggested last time

yes, I agree that we have to store the chain id somewhere in the jumpstart link to avoid unnecessary looping over all supported networks.
should we have a separate task for that?

in the meanwhile we can assume that we get network id from the link, right?

yes let's do that! since we create the link when the user makes the transaction, we'll already have this information and no reason why we can't add it to the link to make our lives easier

let's make the network id pathParts[3] of the jumpstart link

jeanregisser · 2024-02-28T10:58:34Z

src/jumpstart/slice.ts

+  showLoading: boolean
+  showError: boolean


Just checking, do we need both loading and error state at the same time?
If not, I'd advise to model it differently to avoid impossible states.

For instance:

wallet/src/positions/slice.ts

Line 6 in 2b89723

type Status = 'idle' | 'loading' | 'success' | 'error'

good point, thanks! refactored it to have just one consistent property:

export interface State { claimStatus: 'idle' | 'loading' | 'error' }

kathaypacific

looks great!! a few non blocking annoying comments, do with them what you want :)

kathaypacific · 2024-02-28T12:32:19Z

src/jumpstart/jumpstartLinkHandler.test.ts

@@ -43,27 +39,23 @@ describe('jumpstartLinkHandler', () => {
  it('calls executeClaims with correct parameters', async () => {
    ;(fetchWithTimeout as jest.Mock).mockImplementation(() => ({
      ok: true,
+      json: async () => ({ transactionHash: '0xTEST' }),


nit: maybe this can be different from the contractAddress?

kathaypacific · 2024-02-28T12:42:09Z

src/app/saga.ts

@@ -298,12 +299,6 @@ export function* handleDeepLink(action: OpenDeepLink) {
  const { isSecureOrigin } = action
  Logger.debug(TAG, 'Handling deep link', deepLink)

-  const walletAddress = yield* select(walletAddressSelector)


nit: i wonder if we should leave this here and continue to expect the wallet address as a param in the jumpstartClaim saga. only the jumpstartClaim saga is technically needing the wallet address right now, so i understand why you made the change.

it's been annoying in the past for deep links to be processed when wallet address is not available. for example, the opened_via_invite_url event ended up being unhelpful because the walletAddress was undefined and we couldn't match inviters and invitees. the logic in the wallet has changed now where this saga shouldn't be triggered unless the walletAddress is available, but that is done in a hook and perhaps nice to fail early here if something goes wrong?

but failing early will mean we don't show the user any feedback about the error, right?

we could add a generic alert, but failing inside the specific handler means we could communicate what specific situation we are processing, and what error we've encountered

hmm yes, i guess i can see it both ways.

failing early here = we assert all deep links should be handled after the wallet is initialized and it does not matter whether the deep link strictly needs the wallet address in the logic. even if the logic succeeds, our instrumentation will be broken becuase the wallet address is missing.

failing contextually = we have more information about where it failed, but we will allow silent instrumentation failures.

i have a preference for the first scenario, because i cannot think of a scenario where we'd want to process a deep link when a wallet is not initialised?

I agree. There may be a logic requiring a wallet address not directly in a deep link handler, down the user journey, like when link handler fires a navigate event.

kathaypacific · 2024-02-28T12:45:15Z

src/jumpstart/saga.ts

+
+      const token = tokensById[tokenId]
+      if (!token) {
+        Logger.error(TAG, 'Claimed unknown tokenId', tokenId)


Suggested change

Logger.error(TAG, 'Claimed unknown tokenId', tokenId)

Logger.warn(TAG, 'Claimed unknown tokenId', tokenId)

seems that we wouldn't want to investigate this right?

kathaypacific · 2024-02-28T12:46:19Z

src/jumpstart/saga.ts

+          tokenId: tokenId.toString(),
+        })
+      } catch (error) {
+        Logger.error(TAG, 'Error adding pending NFT transaction', error)


same here, i wonder if we should downgrade these to .warn since the pending transactions is a nice to have visual feedback rather than core functionality?

### Description 1. Introducing new slice to the redux state for jumpstart link handling: ``` jumpstart: { showLoading: boolean showError: boolean } ``` This state is expected to be used for rendering UI components displaying the jumpstart link handling state to the user. The actual components will follow in the subsequent PR once the design discussions converge. 2. After successful jumpstart claim two types of pending transactions are dispatched (getting tx data trough blockchain RPC calls): * Payment received (for ERC-20 claim) * NFT received (for ERC-721 claim) Those transactions are expected to be replaced with blockchain-api data once it's fetched. 3. Added analytics events for jumpstart handler lifecycle: * claim succeeded * claim failed * ERC20 token claimed * ERC721 nft claimed ### Test plan * Updated unit tests ### Related issues - Related to RET-1004 ### Backwards compatibility Y ### Network scalability If a new NetworkId and/or Network are added in the future, the changes in this PR will: - [x] Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

bakoushin added 17 commits February 23, 2024 10:05

do it

1f2a922

saga unit tests

9f2f451

reducer unit tests

4bac709

fix jumpstartLinkHandler test

3a31a99

fix app/saga test

6ad9f07

tidy up

3359c7d

move jumpstart state to its own slice

62af43d

fix animation handler

ce149ed

fix toast layout

b334ac4

fix animation freeze

3e7ba03

tune naming

796add5

temporarily remove UI components

a43d0f8

add selectors unit tests

49f3117

tidy up

9cad081

temporarily remove translations

e34037a

fix typing issues

c4fa12e

update root schema

092d19b

bakoushin mentioned this pull request Feb 23, 2024

chore: update wallet jumpstart abi format #4966

Merged

kathaypacific reviewed Feb 26, 2024

View reviewed changes

bakoushin added 5 commits February 26, 2024 13:10

wrap executeClaims to Promise.all

014149a

discard jumpstart_started event

f2df9d6

refactor for multichain support

fff84af

Merge branch 'main' into alex/jumpstart-handler-state

232597c

tidy up

bf7a960

bakoushin added 2 commits February 27, 2024 10:20

track successful token/nft claims with specific events

63fbdca

log error when claiming unknown token

cbedced

blacklist jumpstart state

b339ec4

Merge branch 'main' into alex/jumpstart-handler-state

b132710

kathaypacific reviewed Feb 27, 2024

View reviewed changes

bakoushin added 3 commits February 27, 2024 16:06

discard error logging within the networkId loop

c033b97

use networkId from the jumpstart link

823cb17

Merge branch 'main' into alex/jumpstart-handler-state

5da80ce

jeanregisser reviewed Feb 28, 2024

View reviewed changes

store claim status within one consistent property

59c2e15

kathaypacific approved these changes Feb 28, 2024

View reviewed changes

bakoushin added 4 commits February 28, 2024 16:27

downgrade pending txs dispatch errors to warn

807f897

update test case

2c218e7

restore an early walletAddress check

23d6592

Merge branch 'main' into alex/jumpstart-handler-state

86e4e85

bakoushin enabled auto-merge March 1, 2024 10:52

bakoushin added this pull request to the merge queue Mar 1, 2024

Merged via the queue into main with commit 7672934 Mar 1, 2024
16 checks passed

bakoushin deleted the alex/jumpstart-handler-state branch March 1, 2024 11:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(jumpstart): handle jumpstart state #4963

feat(jumpstart): handle jumpstart state #4963

bakoushin commented Feb 23, 2024 •

edited

Loading

codecov bot commented Feb 23, 2024 •

edited

Loading

kathaypacific left a comment

kathaypacific Feb 26, 2024

bakoushin Feb 27, 2024 •

edited

Loading

bakoushin Feb 27, 2024 •

edited

Loading

kathaypacific Feb 26, 2024

kathaypacific Feb 26, 2024

bakoushin Feb 27, 2024 •

edited

Loading

emerge-tools bot commented Feb 26, 2024 •

edited

Loading

bakoushin commented Feb 27, 2024

kathaypacific left a comment

kathaypacific Feb 27, 2024

bakoushin Feb 27, 2024

kathaypacific Feb 27, 2024

bakoushin Feb 27, 2024

kathaypacific Feb 28, 2024

kathaypacific Feb 28, 2024

jeanregisser Feb 28, 2024

bakoushin Feb 28, 2024 •

edited

Loading

kathaypacific left a comment

kathaypacific Feb 28, 2024

kathaypacific Feb 28, 2024

bakoushin Feb 28, 2024 •

edited

Loading

kathaypacific Feb 29, 2024

bakoushin Mar 1, 2024

kathaypacific Feb 28, 2024

kathaypacific Feb 28, 2024

	Logger.error(TAG, 'Claimed unknown tokenId', tokenId)
	Logger.warn(TAG, 'Claimed unknown tokenId', tokenId)

feat(jumpstart): handle jumpstart state #4963

feat(jumpstart): handle jumpstart state #4963

Conversation

bakoushin commented Feb 23, 2024 • edited Loading

Description

Test plan

Related issues

Backwards compatibility

Network scalability

codecov bot commented Feb 23, 2024 • edited Loading

Codecov Report

kathaypacific left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakoushin Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

bakoushin Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakoushin Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

emerge-tools bot commented Feb 26, 2024 • edited Loading

1 build increased size

Celo (test) 1.79.0 (144)org.celo.mobile.test

bakoushin commented Feb 27, 2024

kathaypacific 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

bakoushin Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

kathaypacific left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakoushin Feb 28, 2024 • 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

bakoushin commented Feb 23, 2024 •

edited

Loading

codecov bot commented Feb 23, 2024 •

edited

Loading

bakoushin Feb 27, 2024 •

edited

Loading

bakoushin Feb 27, 2024 •

edited

Loading

bakoushin Feb 27, 2024 •

edited

Loading

emerge-tools bot commented Feb 26, 2024 •

edited

Loading

Celo (test) 1.79.0 (144)
org.celo.mobile.test

bakoushin Feb 28, 2024 •

edited

Loading

bakoushin Feb 28, 2024 •

edited

Loading