Basic :: Network Package :: Driver #176
Conversation
add example test
Test CoverageSummary
|
packages/network/src/driver/cache.ts
Outdated
| type Slot = ThorStatus['head'] & { | ||
| bloom?: ReturnType<typeof bloomUtil.distribute>; // Bloom filter for the block. | ||
| block?: Block; // Holds a reference to a blockchain block if available. | ||
| accounts: Map<string, Account>; // Map of accounts. | ||
| txs: Map<string, Transaction>; // Map of transactions. | ||
| receipts: Map<string, TransactionReceipt>; // Map of transaction receipts. | ||
| tied: Map<string, unknown>; // Map of tied objects. | ||
| }; |
There was a problem hiding this comment.
Define for each driver file a dedicated folder (e.g., for cache.ts a folder named cache).
Inside that folder you'll have cache.ts, types.d.ts & index.ts.
Define types respectively
In the types folder keep shared types between components
Let's keep our codebase structuring 🎖️
packages/network/src/driver/cache.ts
Outdated
| /** | ||
| * Constant that defines the window length used for determining irreversibility. | ||
| */ | ||
| const WINDOW_LEN = 12; |
There was a problem hiding this comment.
I would work with a dedicated consts folder like core package
packages/network/src/driver/cache.ts
Outdated
| * Represents a Slot in the cache, combining information about a block, accounts, transactions, receipts, and a tied map. | ||
| */ | ||
| type Slot = ThorStatus['head'] & { | ||
| bloom?: ReturnType<typeof bloomUtil.distribute>; // Bloom filter for the block. |
packages/network/src/driver/cache.ts
Outdated
| bloom: | ||
| bloom != null | ||
| ? bloomUtil.distribute( | ||
| bloomUtil.hash( | ||
| Buffer.from(bloom.bits.slice(2), 'hex') | ||
| ), | ||
| bloom.k, | ||
| 2, // ?? | ||
| (index, bit) => bit === index | ||
| ) | ||
| : undefined, |
There was a problem hiding this comment.
This is how we generate a bloom filter given bloom and k:
const bloomBuffer = Buffer.from(dataUtils.removePrefix(bloom), 'hex');
const bloomFilter = new bloomInstance.Filter(bloomBuffer, k);
For more details check bloom utils
There was a problem hiding this comment.
In this case no need to removePrefix as you are getting the bloom bits. Convert to hex and create the filter
| /** Wallet interface manages private keys */ | ||
| interface Wallet { | ||
| /** list all keys */ | ||
| readonly list: WalletKey[]; |
There was a problem hiding this comment.
WalletKey sounds like an access key.
| @@ -46,7 +48,7 @@ class SimpleNet implements Net { | |||
| method: 'GET' | 'POST', | |||
| path: string, | |||
| params?: NetParams | |||
| ): Promise<unknown> { | |||
| ): Promise<Block | undefined> { | |||
There was a problem hiding this comment.
Why is http now constrained to only returning a Block?
| * Retrieves the block based on the provided revision. | ||
| * @returns A promise that resolves to the requested block or null if not found. | ||
| */ | ||
| get: () => Promise<Block | null | undefined>; |
| sign: ((kind: 'tx', msg: VendorTxMessage) => VendorTxSigningService) & | ||
| ((kind: 'cert', msg: VendorCertMessage) => VendorCertSigningService); |
There was a problem hiding this comment.
Same here, extract the function signatures. Let's keep things as simple as possible for readability and maintenance
| interface VMClause { | ||
| to: string | null; // Recipient address (or null). | ||
| value: string | number; // Value in hex string or number. | ||
| data?: string; // Transaction data in hex string (optional). | ||
| } |
There was a problem hiding this comment.
This data type is being duplicated around the codebase. It refers to TransactionClause. Due to the independent nature of packages, I would create a new type in network for the transaction clause but we must keep it with same naming otherwise we create confusion
| @@ -44,12 +44,17 @@ class DriverNoVendor { | |||
| * @returns A promise that resolves to the requested block. | |||
| */ | |||
| public async getBlock( | |||
| revision: string | number | |||
| revision: string | number, | |||
| useCache?: boolean | |||
There was a problem hiding this comment.
@fabiorigam may be better to put this in at the constructor level, and reference that across all functions
There was a problem hiding this comment.
Thanks for the feedback. We decided we won't implement cache in the first release. And we're thinking another way to better implement driver from scratch.
|
We decided to implement driver from scratch in another way |
I still have to work on tests and improve code, but meanwhile I would like a feedback.