Resource accounting implementation #1898
Conversation
corrideat
changed the title
Passing run #2038 ↗︎
Details:
Review all test suite changes for PR #1898 ↗︎ |
|||||||||||||||
|
Removed dependencies detected. Learn more about Socket for GitHub ↗︎ 🚮 Removed packages: npm/@chelonia/[email protected] |
corrideat
force-pushed
the
accounting-implementation
branch
from
a8c6997 to
f6dcac8
Compare
corrideat
force-pushed
the
accounting-implementation
branch
from
f6dcac8 to
63c6642
Compare
corrideat
force-pushed
the
accounting-implementation
branch
from
14b15bd to
7fb549a
Compare
| @@ -287,6 +355,91 @@ route.GET('/file/{hash}', { | |||
| return h.response(blobOrString).etag(hash) | |||
| }) | |||
|
|
|||
| route.POST('/deleteFile/{hash}', { | |||
There was a problem hiding this comment.
Can you add a comment above this line something to the effect of, "allow file deletion, and allow either the bearer of the deletion token or the file owner to delete it" ?
| await sbp('chelonia/db/delete', hash) | ||
| await sbp('chelonia/db/delete', `_private_owner_${hash}`) | ||
| await sbp('chelonia/db/delete', `_private_size_${hash}`) |
There was a problem hiding this comment.
Should these also be done atomically in a queue? If not, can you add a comment explaining why there's an exception for them?
| // b is a hash of a random public key (`g^r`) with secret key `r`, | ||
| // which is used by the requester to commit to that particular `r` | ||
| b: Joi.string().required() | ||
| }, | ||
| { | ||
| r: Joi.string().required(), // what r is | ||
| s: Joi.string().required(), // what s is | ||
| // `r` is the value used to derive `b` (in this case, it's the public | ||
| // key `g^r`) | ||
| r: Joi.string().required(), | ||
| // `s` is an opaque (to the client) value that was earlier returned by | ||
| // the server | ||
| s: Joi.string().required(), | ||
| // `sig` is an opaque (to the client) value returned by the server | ||
| // to validate the request (ensuring that (`r`, `s`) come from a | ||
| // previous request | ||
| sig: Joi.string().required(), | ||
| // `Eh` is the Eh = E_{S_A + S_C}(h), where S_A and S_C are salts and | ||
| // h = H\_{S_A}(P) |
There was a problem hiding this comment.
Nice!! Thanks for updating these! 😄
| // This ensures that only the owner of the contract can set a salt for it, | ||
| // closing a small window of opportunity(*) during which an attacker could | ||
| // potentially lock out a new user from their account by registering a | ||
| // different salt. | ||
| // (*) This is right between the moment an OP_CONTRACT is sent and the | ||
| // time this endpoint is called, which should follow almost immediately after. |
| { | ||
| id: SAKid, | ||
| name: '#sak', | ||
| purpose: ['sak'], | ||
| ringLevel: 0, | ||
| permissions: [], | ||
| allowedActions: [], | ||
| meta: { | ||
| private: { | ||
| content: SAKs | ||
| } | ||
| }, | ||
| data: SAKp |
There was a problem hiding this comment.
Shouldn't we make sure that the #sak has proper permissions before the server allows it to be registered?
I.e. verify ringLevel: 0 and permissions: []?
There was a problem hiding this comment.
Possibly, although this isn't the place to check that.
| } | ||
| // If the request didn't succeed, report it | ||
| if (!time.ok) throw new Error('Error fetching server time') | ||
| const serverTime = (new Date(await time.text())).valueOf() |
There was a problem hiding this comment.
If it takes 200 ms for our request to get to the server, and 200ms to get the response, by the time we get the response from the server, the time we receive from the server will be 200ms old. So we should add 200ms to the time received from the server.
We can estimate this time by doing (requestTimeElapsed - newMonotonicBase) / 2
shared/domains/chelonia/time-sync.js
Outdated
| let wallBase = Date.now() | ||
| let monotonicBase = performance.now() |
There was a problem hiding this comment.
Could you add comments here explaining what wallBase and monotonicBase mean? These terms might be confusing to people (as they are confusing to me)
| // Tolerate up to a 10ms difference | ||
| if (difference > 10) { |
There was a problem hiding this comment.
10ms seems rather small... how often would this get called?
There was a problem hiding this comment.
This is called locally and the difference should typically be 0, unless the local date has been adjusted for some reason.
| } | ||
|
|
||
| export default (sbp('sbp/selectors/register', { | ||
| 'chelonia/private/startClockSync': function () { |
There was a problem hiding this comment.
How difficult would it be to adapt this code to support multiple different servers? For federation it would be necessary to be sync'd to each server on which we are monitoring/interacting with contracts.
There was a problem hiding this comment.
For this particular code, probably not so much. The issue is that currently there's only a single connectionURL. The changes needed here would depend on how multiple server URLs are handled.
No description provided.