Replace local hubble-bls fork with bls-wallet-signer #55
Conversation
src/chain/BlsWallet.ts
Outdated
| await parent.getChainId(), | ||
| this.#VerificationGateway(parent), | ||
| this.#Signer(secret), | ||
| static async connectOrCreate(privateKey: string, parent: ethers.Wallet) { |
There was a problem hiding this comment.
Add comments for BLSWallet class, including params of functions when helpful for clarity, eg parent
There was a problem hiding this comment.
BLSWallet class could be used without bls private keys and signing. PKs and signing could be exposed/used specifically, namely for testing and -extension.
There was a problem hiding this comment.
Add comments for BLSWallet class, including params of functions when helpful for clarity, eg
parent
👍 Updated
BLSWallet class could be used without bls private keys and signing. PKs and signing could be exposed/used specifically, namely for testing and -extension.
Yeah I think there will be some more changes here and maybe in bls-wallet-signer. I figure they will be made as they are needed though. Do you have specific changes in mind for right now?
src/app/TxTable.ts
Outdated
| // This will cause problems from 2^31 because we're using 32 bit integers. | ||
| // It's also a problem from 2^53 for js numbers. | ||
| // | ||
| // More information: https://github.com/jzaki/aggregator/issues/36. |
There was a problem hiding this comment.
Update to reference issue number #36
There was a problem hiding this comment.
I'm confused, it does reference issue #36:
// More information: https://github.com/jzaki/aggregator/issues/36.
Could you clarify?
There was a problem hiding this comment.
This was a minor one, just the #36is required because the full link name has changed (although github does redirect successfully)
There was a problem hiding this comment.
Oh I see. Updated.
|
|
||
| import assertExists from "../helpers/assertExists.ts"; | ||
| import nil from "../helpers/nil.ts"; | ||
|
|
||
| export type TransactionData = { | ||
| type RawTxTableRow = { |
There was a problem hiding this comment.
Consistency with transaction parameter order (outer-most to inner-most)
There was a problem hiding this comment.
👍 (pending update)
(+ discussed on Signal)
| [hubbleBls.mcl.loadG2(pubkey)], | ||
| )); | ||
| } | ||
| import { TxTableRow } from "./TxTable.ts"; |
There was a problem hiding this comment.
Can use TransactionData instead of TxTableRow in WalletService? txIds only used for emitting events
There was a problem hiding this comment.
Hmm I don't think there is a nice way to do this. I'm not sure how 'only for events' helps. Did you have something other than casting in mind?
There was a problem hiding this comment.
TransactionData has txId added here. Should the wallet service only worry about TransactionData rather than table rows (TxTableRow)?
src/app/TxTable.ts
Outdated
| } | ||
|
|
||
| async find(pubKey: string, nonce: number): Promise<TransactionData | nil> { | ||
| async find(publicKey: string, nonce: BigNumber): Promise<TxTableRow | nil> { |
There was a problem hiding this comment.
Consider renaming to findSingle (findFirst?) since results limited to 1
There was a problem hiding this comment.
👍 (pending update)
| import { TransactionData } from "./TxTable.ts"; | ||
| import { BigNumber, TransactionData } from "../../deps.ts"; | ||
|
|
||
| export type TransactionDataDTO = { |
There was a problem hiding this comment.
Data Transfer Object
test/TxServiceBatching.test.ts
Outdated
| ); | ||
|
|
||
| assertEquals(await fx.allTxs(txService), { | ||
| ready: [], | ||
| future: [], | ||
| }); | ||
|
|
||
| console.log("here"); |
There was a problem hiding this comment.
👍 (pending update)
Things removed:
TxTable.ts)Notable changes:
TransactionDatatype from bls-wallet-signer which doesn't know about the txId field which is a database concern. So, now we have a distinguished typeTxTableRowwhich is justTransactionDatawith an optionaltxId.BigNumbernow, there's aTransactionDataDTOtype to support sending this as JSON by representing those fields as strings. The client still accepts a regularTransactionData, the DTO is internal and only needs to be handled byClient.ts,parsers.ts, andTxRouter.ts../programs/premerge.tswhich checks all the typescript code, does linting, runs tests in parallel, and checks for todos & fixmesSorry about the volume of superficial edits to accomplish the above. Most of it is in test code.