Skip to content
This repository has been archived by the owner on Jan 24, 2023. It is now read-only.

Added a revision number cache to make setJSON safer #131

Merged
merged 38 commits into from
Dec 17, 2021

Conversation

redsolver
Copy link
Contributor

@redsolver redsolver commented May 28, 2021

src/skydb.ts Outdated Show resolved Hide resolved
src/skydb.ts Outdated Show resolved Hide resolved
src/skydb.ts Outdated Show resolved Hide resolved
src/skydb.ts Outdated Show resolved Hide resolved
@ro-tex
Copy link
Collaborator

ro-tex commented Aug 5, 2021

Is this PR still alive? Let's either close it or push it across the finish line.

- Refactor SkyDB helpers to use revision cache and rename them
- Make all SkyDB methods use revision cache
- Make MySky DB methods call SkyDB methods or helpers
- Update SkyDB tests
integration/registry.test.ts Outdated Show resolved Hide resolved
src/mysky/index.ts Outdated Show resolved Hide resolved
integration/skydb.test.ts Outdated Show resolved Hide resolved
@mrcnski
Copy link
Contributor

mrcnski commented Nov 10, 2021

We are probably going to change the spec, and I'm still adding tests.

@mrcnski mrcnski marked this pull request as draft November 10, 2021 00:37
- [X] Integration test: setJSON and getJSON called simultaneously
- [X] Unit test: setJSON and getJSON called simultaneously
- [X] Integration test: setJSON and getJSON called with a delay
- [X] Integration test: setJSON and getJSON called with two different clients (different local caches)
- [X] Unit test: setJSON and getJSON called with two different clients (different local caches)
- [X] Unit test: multiple setJSON calls where one fails
peterjan
peterjan previously approved these changes Dec 13, 2021
ChrisSchinnerl
ChrisSchinnerl previously approved these changes Dec 13, 2021
mrcnski and others added 8 commits December 13, 2021 11:20
Bumps [typescript](https://github.com/Microsoft/TypeScript) from 4.5.2 to 4.5.3.
- [Release notes](https://github.com/Microsoft/TypeScript/releases)
- [Commits](microsoft/TypeScript@v4.5.2...v4.5.3)

---
updated-dependencies:
- dependency-name: typescript
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [eslint-plugin-jsdoc](https://github.com/gajus/eslint-plugin-jsdoc) from 37.1.0 to 37.2.0.
- [Release notes](https://github.com/gajus/eslint-plugin-jsdoc/releases)
- [Commits](gajus/eslint-plugin-jsdoc@v37.1.0...v37.2.0)

---
updated-dependencies:
- dependency-name: eslint-plugin-jsdoc
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Previously, methods were returning error messages that said e.g. "Request failed
with status code 429" and we had to drill down into the actual response to see
what the skyd error was.

All methods that make a request now return a descriptive error message based on
the skyd response message.

- Added ExecuteRequestError which contains the original Axios error

- Added request.ts file for ExecuteRequestError, where it was causing circular
  dependencies and stuff not being defined at runtime.

- Also, I fixed a bug where a promise in `uploadLargeFile` was not being
  rejected on error.
Previously, the returned signature was longer than `SIGNATURE_LENGTH`. This is
because `sign` returns the signed message (including the original message) which
was not necessary -- we only need it to return the signature.

In addition to being unnecessary, it was also confusing to have a signature that
was longer than expected.

We should have been using `sign.detached`. We already use `verify.detached`.
@mrcnski mrcnski dismissed stale reviews from ChrisSchinnerl and peterjan via b840ef1 December 13, 2021 17:21
Copy link

@peterjan peterjan left a comment

Choose a reason for hiding this comment

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

Looks good, would approve if it weren't for the failing test. Seeing as we introduce locking and the test is timing out it warrants investigation as it might be a deadlock, although unlikely.

@mrcnski mrcnski merged commit 65e7b1c into SkynetLabs:master Dec 17, 2021
@mrcnski mrcnski mentioned this pull request Jan 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants