Skip to content
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

Improve capabilities updates and transport syncing order #2838

Merged
merged 6 commits into from
Jun 30, 2021

Conversation

andrevmatos
Copy link
Contributor

@andrevmatos andrevmatos commented Jun 29, 2021

Fixes #2831

Short description
We implemented the workaround to quickly toggle our transport status between offline and back online to trigger PFS to be informed of our new capabilities. Additionally, some needed fixes went into transport code handling capabilities updates: udcDepositEpic won't push outdated balance when mining the transaction, which did cause a wrong udcDeposit.success update and respective Receive=0 capability to be set for a short period of time. Also, transport now is prepared before but started only when synced, to only then appear as online and start receiving transport events. Finally, some previous presence workaround tries were cleaned up, since they don't make sense anymore with PFS-based presence mechanism.

Definition of Done

  • Steps to manually test the change have been documented
  • Acceptance criteria are met
  • Change has been manually tested by the reviewer (dApp)

Steps to manually test the change (dApp)

  1. Have capabilities change while client is running (e.g. by UDC depositing or withdrawing across monitoringReward)
  2. Check it got updated in curl -vL https://pfs-goerli-with-fee.services-dev.raiden.network/api/v1/address/<address>/metadata

When depositing to UDC, after the tx just got mined, it emits a
udcDeposit.success. So far, it was being emitted with the value it just
got from a new effectiveBalance call, which may be outdated because the
tx didn't get confirmed yet. Instead, we now calculate the actual "final"
effectiveBalance to be used there since we know it from previous
balance.
@andrevmatos andrevmatos self-assigned this Jun 29, 2021
@codecov
Copy link

codecov bot commented Jun 29, 2021

Codecov Report

Merging #2838 (d6d7451) into master (3f8e74b) will decrease coverage by 0.10%.
The diff coverage is 96.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2838      +/-   ##
==========================================
- Coverage   93.68%   93.58%   -0.11%     
==========================================
  Files         110      110              
  Lines        6038     6030       -8     
  Branches     1093     1093              
==========================================
- Hits         5657     5643      -14     
- Misses        326      332       +6     
  Partials       55       55              
Flag Coverage Δ
dapp 80.64% <ø> (ø)
dapp.unit 80.64% <ø> (ø)
sdk 95.75% <96.96%> (-0.13%) ⬇️
sdk.e2e 74.66% <90.90%> (-0.08%) ⬇️
sdk.integration 78.43% <96.96%> (-0.15%) ⬇️
sdk.unit 50.32% <15.15%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
raiden-ts/src/transport/epics/init.ts 98.05% <92.85%> (-1.04%) ⬇️
raiden-ts/src/epics.ts 98.98% <100.00%> (+0.01%) ⬆️
raiden-ts/src/services/epics/udc.ts 100.00% <100.00%> (ø)
raiden-ts/src/transport/epics/presence.ts 100.00% <100.00%> (ø)
raiden-ts/src/transport/epics/webrtc.ts 93.49% <0.00%> (-2.04%) ⬇️

Continue to review full report at Codecov.

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

ensures there's some time after raiden start before updating
capabilities on matrix for the first time; also, matrix gets started
only after raiden is synced, to avoid receiving messages while we're not
ready yet.
Do this only after we're synced and matrix is started. To force this
update, we quickly appear offline then back online, although syncing
isn't interrupted.
Copy link
Contributor

@weilbith weilbith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The associated issue should be solved with this PR. I'll test it now and then
give my approval. I just have some question to the code itself. No critics, just
questions to understand the logic myself.
Good job! 👍

raiden-ts/src/services/epics/udc.ts Show resolved Hide resolved
raiden-ts/src/services/epics/udc.ts Show resolved Hide resolved
raiden-ts/src/services/epics/udc.ts Show resolved Hide resolved
raiden-ts/src/transport/epics/presence.ts Show resolved Hide resolved
@weilbith
Copy link
Contributor

Now I just approved... 😆
I'll test it anyway now.

@weilbith
Copy link
Contributor

Works like charm. 😊
One thing I recognized: we do update our capabilities already when the tx got mined, but do not wait until it get confirmed. Is this intended?

@andrevmatos
Copy link
Contributor Author

Works like charm. 😊
One thing I recognized: we do update our capabilities already when the tx got mined, but do not wait until it get confirmed. Is this intended?

Good eyes. Yes, it is, mainly because it'd be hard to know when balance changed got confirmed on the monitoring epic, because there there's no txHash. Also, there's nowhere depending on this being confirmed, it's just something we depend (so we can enable receiving or not), so there's nothing to get out of sync if this is still unconfirmed.

@weilbith
Copy link
Contributor

What happens in case the transaction gets reorged? Would this reset the receiving capability again?

@andrevmatos
Copy link
Contributor Author

What happens in case the transaction gets reorged? Would this reset the receiving capability again?

In case of reorgs, the balance would be updated as soon as the reorg got detected and the capability would immediately reflect this change, if needed. Nonetheless, I just pushed a small change to ensure we only set udcDeposits for confirmed balances, so hopefully we'll be on the safe side for this.

Copy link
Contributor

@weilbith weilbith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the last improvement. I can confirm that it now works as expected. 👍🏾

@andrevmatos andrevmatos merged commit 07b0c2f into master Jun 30, 2021
@andrevmatos andrevmatos deleted the improv/caps_updates branch June 30, 2021 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dApp: Direct transfer fails with "The Requested target doesn't receive transfers"
2 participants