feat: add basic persistence, fixes #19
Conversation
kincaidoneil
commented
Mar 5, 2019
•
kincaidoneil
commented
- Add basic persistence to config file in user directory
- Fix XRP issues: Withdrawing from XRP uplink requires 2 synchronous on-chain txs #16, XRP _autoClaim fails if no claim exists (withdraw & remove fail) #9; temporary fix for Funding less than minIncoming on XRP prevents any incoming capacity #15
- Fix issue with balance not applied for incoming packets
- Resolves ETH issues: Deposits on ETH open channels for 0 amounts #7, bug loading persisted claims (other issues were on server)
- Refactor some test code
- prettier should also format js files - tslint shouldn't auto-fix
- pro: error messages/stack traces are much clearer - con: code coverage wasn't working right, but hopefully this fixes it
- fix linting errors/add readonly properties - remove "settlement module" interface - add wip commented out encryption code
- middlewares no longer implements full plugin interface since it's not necesary for now
- more concise helpers - acknowledge failing tests (temporarily) - delete config after each test
- no logs in ci triggers timeout errors when multiple funding txs are required
- temporarily addresses #15, at least for the Kava connector's config - after deposit, refresh outgoing channel amount slightly later to ensure the channel exists
- remove unused function - export util from lnd
- on Linux, positional writes aren't supported in append mode - r+ mode doesn't overwrite file content by default
Codecov Report
@@ Coverage Diff @@
## master #19 +/- ##
=========================================
Coverage ? 89.72%
=========================================
Files ? 16
Lines ? 662
Branches ? 35
=========================================
Hits ? 594
Misses ? 59
Partials ? 9
Continue to review full report at Codecov.
|
src/config.ts
Outdated
| credentials: state.credentials.map(credentialToConfig) | ||
| }) | ||
|
|
||
| export const persistConfig = async (fd: number, state: State) => { |
There was a problem hiding this comment.
It's unlcear to me what fd is and what values it will take on depending on what happens. Maybe comment to clarify or change the name.
| }, | ||
| credentials: [], | ||
| uplinks: [] | ||
| } | ||
|
|
||
| // TODO Add functionality to connect existing uplinks based on config | ||
| // (unnecessary/backburner until persistence is added) | ||
| // TODO Handle error cases if the uplinks fail to connect |
There was a problem hiding this comment.
To clarify, what happens currently if an uplink being restored fails to connect (say if the connector is offline)?
There was a problem hiding this comment.
It hangs indefinitely...
src/settlement/machinomy.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * Ensure that the given string is begins with given prefix |
There was a problem hiding this comment.
typo: string is begins -> string begins
There was a problem hiding this comment.
Overall this looks good to me, nice test refactor.
I think before adding encryption it might be worth looking at storing stuff in some key-value store (LevelDB maybe?). I think the change is really minor and it could fix a few problems while making the code nicer: 'instant' instead of timed persistence, encryption done for us, compatibility between windows/mac/linux file systems, ability to switch to an in memory store if needed.
src/__tests__/disconnect.test.ts
Outdated
| t.context = await connect(process.env.LEDGER_ENV! as LedgerEnv) | ||
| }) | ||
|
|
||
| test('after connect', async t => { |
There was a problem hiding this comment.
What was the reason for removing this test?
src/settlement/machinomy.ts
Outdated
| * Ensure that the given string is begins with given prefix | ||
| * - Prefix the string if it doesn't already | ||
| */ | ||
| const prefixWith = (prefix: string, str: string) => |
There was a problem hiding this comment.
rename to ensurePrefixedWith?
src/index.ts
Outdated
| } | ||
|
|
||
| export const connect = async (ledgerEnv: LedgerEnv = LedgerEnv.Testnet) => { | ||
| let state: State = { | ||
| const [fd, config] = await loadConfig() |
There was a problem hiding this comment.
I think we shouldn't hard code the directory where data is stored.
Maybe add a dataDir: string arg to connect and loadConfig. Then set a default in connect to home/.switch/.
|
@rhuairahrighairidh I opened #23 and #24 to track the persistence improvements |
