-
Notifications
You must be signed in to change notification settings - Fork 478
Update to RecordProvider Interface #1074
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
Update to RecordProvider Interface #1074
Conversation
564eefa
to
c927c95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good core impl, but several comments on the interface, code organization, and some method naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking better, the following things should be added
- Can we add doc comments to the interface types. People will be confused about this initially and docs will help.
- Can you add tests of the default implementation against stubs?
- A few more nits?
e5f7a46
to
6427fa2
Compare
sdk/src/record-scanner.ts
Outdated
* | ||
* const records = await recordScanner.findRecords({ filter, responseFilter }); | ||
*/ | ||
class RecordScanner implements RecordProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before it's marked ready this should pass tests using stubs for the test data.
dba689e
to
7f88144
Compare
7f88144
to
cdd92fa
Compare
b78fbbb
to
4487169
Compare
4487169
to
de37e1c
Compare
de37e1c
to
8afa6ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some (expected) changes to the interface added here.
Secondly, let's please split out the concrete implementation of the RecordScanner from this PR and make it PR into this one. We need to put some pretty significant tests around it to ensure it's production ready. The interface however, matches what we expect.
sdk/src/record-scanner.ts
Outdated
* @param {number} startBlock The block height to start scanning from. | ||
* @returns {Promise<void>} A promise that resolves when the account is registered. | ||
*/ | ||
async register(startBlock: number): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should return a RegistrationResponse
type
interface RegistrationResponse {
uuid: string,
job_id?: string,
status?: string
}
async register(startBlock: number): Promise<void> { | |
async register(startBlock: number): Promise<void> { |
@@ -0,0 +1,52 @@ | |||
/** | |||
* Encrypted Record found on chain. This type provides the record ciphertext and metadata from the ledger that such as the record's name, the program/function that produced it, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Encrypted Record found on chain. This type provides the record ciphertext and metadata from the ledger that such as the record's name, the program/function that produced it, etc. | |
* Encrypted Record found on chain. This type provides the record ciphertext and metadata from the ledger such as the record's name, the program/function that produced it, etc. |
@@ -0,0 +1,54 @@ | |||
/** | |||
* Record owned by a registered view key. This type provides the record ciphertext, record plaintext and metadata from the ledger that such as the record's name, the program/function that produced it, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Record owned by a registered view key. This type provides the record ciphertext, record plaintext and metadata from the ledger that such as the record's name, the program/function that produced it, etc. | |
* Record owned by a registered view key. This type provides the record ciphertext, record plaintext and metadata from the ledger such as the record's name, the program/function that produced it, etc. |
@@ -0,0 +1,24 @@ | |||
import { RecordSearchParams } from "../record-provider/recordSearchParams"; | |||
import { RecordsFilter } from "./recordsFilter"; | |||
import { RecordsResponseFilter } from "../record-provider/recordsResponseFilter"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need another file to handle the OwnedFilter.
sdk/src/record-provider.ts
Outdated
encryptedRecords(recordsFilter: RecordSearchParams, responseFilter: RecordsResponseFilter): Promise<EncryptedRecord[]>; | ||
|
||
/** | ||
* Check if a list of serial numbers exist in the chosen provider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Check if a list of serial numbers exist in the chosen provider | |
* Check if a list of serial numbers exist in the chosen provider. |
sdk/src/record-provider.ts
Outdated
checkSerialNumbers(serialNumbers: string[]): Promise<Record<string, boolean>>; | ||
|
||
/** | ||
* Check if a list of tags exist in the chosen provider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Check if a list of tags exist in the chosen provider | |
* Check if a list of tags exist in the chosen provider. |
sdk/src/record-scanner.ts
Outdated
/** | ||
* RecordScanner is a RecordProvider implementation that uses the record scanner service to find records. | ||
* | ||
* @example | ||
* const account = new Account({ privateKey: 'APrivateKey1...' }); | ||
* | ||
* const recordScanner = new RecordScanner("https://record-scanner.aleo.org"); | ||
* recordScanner.setAccount(account); | ||
* const uuid = await recordScanner.register(0); | ||
* | ||
* const filter = { | ||
* uuid, | ||
* filter: { | ||
* program: "credits.aleo", | ||
* records: ["credits"], | ||
* }, | ||
* responseFilter: { | ||
* program: true, | ||
* record: true, | ||
* function: true, | ||
* transition: true, | ||
* block_height: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we split this record provider out into a second PR so we can write proper tests around it?
sdk/src/record-scanner.ts
Outdated
return records.filter(record => { | ||
const plaintext = RecordPlaintext.fromString(record.record_plaintext ?? ''); | ||
const amount = plaintext.getMember("microcredits").toString(); | ||
return microcreditAmounts.includes(parseInt(amount.replace("u64", ""))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only match extremely specific micro credits amounts, which will match a very small amount of the time. This should ensure that records are found that are larger than the microcredit amounts in the array.
For instance if you have credits records with amounts:
100,
200,
3000,
4000,
30
And your filter specifies [100, 300, 400]
It should choose amounts above those like [100, 3000, 4000] OR [200, 3000, 4000]
…scanner implementation of said interface
… provider method signatures into RecordSearchParams interface. added jsdoc comments to record scanner
…plain key as a string or an object containing the key and the header name
d8f3421
to
3b345ba
Compare
Signed-off-by: Mike Turner <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexpitsikoulis this is approved, merge at will.
…scanner implementation of said interface
Motivation
(Write your motivation here)
Test Plan
(Write your test plan here)
Related PRs
(Link your related PRs here)