Conversation
lcovar
changed the title
OttoAllmendinger
left a comment
OttoAllmendinger
left a comment
There was a problem hiding this comment.
we are on the right track
CONVENTIONS.md
Outdated
| const signedBytes = tx.toBytes(); | ||
|
|
||
| // For parsing — decode instructions | ||
| const parsed = parseTransaction(txBytes, context); |
There was a problem hiding this comment.
actually would prefer if parseTransaction took the tx as an argument here
There was a problem hiding this comment.
parseTransaction takes bytes to keep it decoupled from Transaction, most callers start with bytes from an api anyway
There was a problem hiding this comment.
wouldn't this be a typical flow?
if parseTransaction takes Transaction we only have to decode once
const tx = Transaction.fromBytes(bytes);
const parsed = parseTransaction(tx);
if (validateParsed(parsed, buildParams)) { throw new Error(); }
tx.sign(privkey);
There was a problem hiding this comment.
youse right. udpated, refactor prs for solana/dot coming up after.
CONVENTIONS.md
Outdated
| } | ||
|
|
||
| // For high-level explanation — business logic | ||
| const explained = explainTransaction(txBytes, options); |
There was a problem hiding this comment.
also we probably need to figure out what the distinction between parse and explain is
maybe just remove the explain example here
There was a problem hiding this comment.
agree, removing.
parseTransaction returns a plain data object with decoded transaction data, Transaction.fromBytes is for signing. explain is basically parseTransaction with bitgo business logic on top, derives the type, extracts outputs/inputs, computes fees. its what bitgojs expects for the explain flow
There was a problem hiding this comment.
Maybe it could live outside the wasm packages then and be built around parseTransaction? at least explainTransaction should be in a bitgo-specific subfolder
For wasm-utxo we have a BitGoPsbt class with a parseWithWalletKeys() and both bitgo explainTransaction and parseTransaction are built around that outside the wasm-utxo package.
There was a problem hiding this comment.
Ok, currently for dot/sol i have it here but im convinced, it should live in bitgojs, ill pull it out.
document 7 architectural patterns enforced in code reviews: - prefer Uint8Array, avoid unnecessary base conversions - bigint for monetary amounts (conversions are caller's responsibility) - as const arrays for union types, not magic strings - builders return Transaction objects, not bytes - parsing separate from Transaction (standalone parseTransaction) - use wrapper classes over raw WASM bindings - consistent wrapper API (fromBytes, toBytes, getId, get wasm()) distilled from recurring review comments across wasm-utxo, wasm-solana, and wasm-dot. ref: PR #145 discussion.
2 participants
documents 7 architectural patterns enforced in code reviews across wasm-utxo, wasm-solana, wasm-mps, and future packages.
these are design decisions that could reasonably be made differently but we've standardized on:
distilled from otto's recurring review comments. prevents review churn by making the patterns explicit.
ref: wasm-dot PR #145 discussion about convention alignment