Arbitrum: replace block timeouts with timestamps #3034
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrevmatos
added
sdk 🖥
breaking 💔
andrevmatos
force-pushed
the
arbitrum
branch
2 times, most recently
from
1fcbe13 to
c9251a5
Compare
andrevmatos
force-pushed
the
arbitrum
branch
from
c9251a5 to
2da7076
Compare
andrevmatos
force-pushed
the
arbitrum
branch
2 times, most recently
from
5207ce9 to
45e74b2
Compare
andrevmatos
force-pushed
the
arbitrum
branch
from
45e74b2 to
321b8cd
Compare
weilbith
reviewed
Feb 23, 2022
andrevmatos
force-pushed
the
arbitrum
branch
3 times, most recently
from
0ca594f to
6ba4ee9
Compare
The former is a cached getter function to fetch the timestamp (in millis) for a specific blockNumber, and the later is a cold cached (shareReplay'd) observable to fetch the contract's constant settleTimeout
fetches from Raiden.settleTimeout getter
Before, when using udcAddress as contract's entrypoint, the discovery had to be performed every session, as it wasn't stored, and also because this contract's discovery was needed to get TNR addr and find state. Now, we use UDC address to index state, and persist the whole discovered contractsInfo in it, so we reuse from state after first init.
We tried some logic with confirmationBlocks and blockTimes before, just as a way to have a more relaxed upperbound so transfers would expire before channel settlement window, but it was quite complex, and this is already a stretched margin anyway (usually, revealTimeout * expiryFactor is used), so we replace that by half settleTimeout.
And avoid additional block wait time of confirmConfirmation (Raiden.openChannel and Raiden.depositChannel) in case confirmationBlocks=0
We had already refactored the epics to check that the timestamp were correct, but still reacted only upon newBlocks. Now, we use a `timer` approach to actually trigger these actions on the deadline/expiration, independently of newBlocks arriving or not
This was awaiting a negative value (secs - now_msecs), so no wait
Also, fix arbitrum-rinkeby network name, and try to read at runtime if deployment not built-in.
In the past, channels had a settle timeout property. It was also possible to configure the SDK with a settle timeout value for new channels. This has changed with the latest version of the contracts and SDK. The settle timeout has moved from individual channels to the token network itself. In result channel data structure have no more this data field and the SDK doesn't accept this configuration option anymore. There is also a visual effect of this change. So far, if a channel got closed, the button to settle the channel was showing a countdown until it becomes possible. But not only was the settle timeout moved, it also changed its meaning from a block number based value to an actual time value in seconds. The issue is that this value can very from a few seconds to multiple days. In result is is not easy to maintain a countdown on the settle button. Also because the rendering of the Vue component would need to be force updated by an interval as it is now purely time based. To maintain the raw functionality for the moment, the button just remains disabled until the channel is settlable. This might improve in future again.
In the past, a planned UDC withdrawal had the property of a target `withdrawBlock` from which it is possible to withdraw the tokens. This has changed to a timestamp value (in Unix format) called `withdrawableAfter`. Due to this change, a couple of renaming where necessary. Furthermore this also has visual effects. On the view to the user deposit contract, if there was a planned withdraw pending (not yet withdrawable) it was showing a countdown for the blocks remaining. As this is not the case anymore, it has been changed to display the target date. Once this date has passed, the UI changes as usual and displays the user that the plan is now ready. There was also a tiny change in the translations, not only according block to time change. Also a special unicode character got replaced with a simple ASCII one, as the former one was not rendered properly for some users, depending on their system font.
This means that basically all related components like contracts, services etc. got updated. As the contracts are not released yet and thereby all dependent components, all versions point to pre-release git tags. There were a few adaption necessary despite exchanging the components which come with changes of these updates. As a side-effect we get a eat-your-own-dogs-food dilemma here. As there is no Raiden client that supports the new contracts yet, we need to fetch the current under-development branch of the Light Client. This must change once a release is ready. The end-to-end tests of the SDK and dApp do not succeed yet. They have not been touched and are likely to fail by syntax and semantic issues.
Also, allow it to not only emit first best PFS, but instead prefer to emit all of them sorted, so caller can take `first` only if desired. This is intended in order to use this functions in other places of the code, like the matrix server choice in transport.
When matrixServer isn't set, we are in `auto` mode. Previously, we'd fetch the list from `matrixServerLookup`, but is easy to have this list mismatched with current network/deployment. This change prefers the same matrixServer from the PFSs, scanning first `config.additionalServices` and then services fetched from ServicesRegistry, which are expected to always be compatible with current deployment.
Codecov Report
@@ Coverage Diff @@
## master #3034 +/- ##
==========================================
- Coverage 93.16% 93.10% -0.07%
==========================================
Files 213 213
Lines 8555 8596 +41
Branches 1428 1407 -21
==========================================
+ Hits 7970 8003 +33
- Misses 511 522 +11
+ Partials 74 71 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
weilbith
approved these changes
Mar 7, 2022
|
@andrevmatos please update the CHANGELOG. |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2916
Short description
Definition of Done
Steps to manually test the change (dApp)