-
Notifications
You must be signed in to change notification settings - Fork 6
feat: ts api jinja generator #250
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
base: main
Are you sure you want to change the base?
Conversation
df8e6df
to
983b927
Compare
packages/typescript/indexer_client/src/core/FetchHttpRequest.ts
Outdated
Show resolved
Hide resolved
b3ed18f
to
6986612
Compare
@neilcampbell note current version duplicates SignedTransaction object from core from future branch on utils - for the time being until its movied to the monorepo |
6986612
to
f0fe799
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.
Pull Request Overview
This PR introduces TypeScript API client generation for Algorand algod and indexer services through a Jinja-based code generator. The implementation creates comprehensive TypeScript clients that support modern features like msgpack encoding, BigInt handling, and request/response transformation.
Key changes:
- Add TypeScript code generation commands for algod and indexer clients
- Implement comprehensive client libraries with typed models and APIs
- Add msgpack support for Algorand-compliant transaction encoding/decoding
- Include manual test suites for client functionality validation
Reviewed Changes
Copilot reviewed 232 out of 238 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
tools/api_tools/src/main.rs | Adds new CLI commands for TypeScript client generation with cleaning, formatting, and building steps |
packages/typescript//src/models/.ts | Generated TypeScript type definitions for all API models with proper BigInt support |
packages/typescript//src/core/.ts | Core HTTP client infrastructure with msgpack, JSON parsing, and request handling |
packages/typescript//src/apis/.ts | Generated API service classes with comprehensive endpoint coverage |
packages/typescript//tests/.ts | Manual test suites for client functionality verification |
Comments suppressed due to low confidence (1)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
fn clean_ts_package_with_preserve(rel_dir: &str, preserve: &[&str]) -> Result<()> { | ||
let root = get_repo_root(); | ||
let pkg_dir = root.join(rel_dir); | ||
if !pkg_dir.exists() { | ||
return Ok(()); | ||
} | ||
|
||
for entry in fs::read_dir(&pkg_dir)? { | ||
let entry = entry?; | ||
let name = entry.file_name(); | ||
let name_str = name.to_string_lossy(); | ||
if preserve.iter().any(|p| *p == name_str) { | ||
continue; | ||
} | ||
let path = entry.path(); | ||
if path.is_dir() { | ||
// Remove entire directory tree | ||
fs::remove_dir_all(&path)?; | ||
} else { | ||
// Remove file | ||
fs::remove_file(&path)?; | ||
} | ||
} | ||
Ok(()) | ||
} |
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.
The function performs destructive file operations without any safety checks or user confirmation. Consider adding validation to ensure the directory being cleaned is within expected bounds and perhaps add a dry-run mode for safety.
Copilot uses AI. Check for mistakes.
clean_ts_package("packages/typescript/algod_client")?; | ||
// Generate the TypeScript client (algod) | ||
run( | ||
"uv run python -m ts_oas_generator.cli ../specs/algod.oas3.json --output ../../packages/typescript/algod_client/ --package-name algod_client --description \"TypeScript client for algod interaction.\" --verbose", |
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.
The command strings are very long and contain hardcoded paths. Consider extracting these into constants or a configuration structure to improve maintainability and reduce duplication across the three similar command blocks.
Copilot uses AI. Check for mistakes.
function prepareForEncoding(value: any): any { | ||
// eslint-disable-line @typescript-eslint/no-explicit-any | ||
if (value === null || value === undefined) { | ||
return undefined; | ||
} | ||
|
||
// Handle numbers - omit zeros | ||
if (typeof value === "number") { | ||
if (value === 0) return undefined; | ||
return value; | ||
} | ||
|
||
// Handle bigints - omit zeros and convert to number when safe (for smaller encoding) | ||
if (typeof value === "bigint") { | ||
if (value === 0n) return undefined; | ||
// Convert to number if it fits safely (implements PositiveIntUnsigned behavior) | ||
if (value <= BigInt(Number.MAX_SAFE_INTEGER) && value >= BigInt(Number.MIN_SAFE_INTEGER)) { | ||
return Number(value); | ||
} | ||
return value; | ||
} |
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.
[nitpick] The zero-omission logic may be too aggressive. In some Algorand contexts, explicit zero values might be semantically meaningful (e.g., zero amount transfers for opt-ins). Consider making this behavior configurable or documenting when zero omission is appropriate.
Copilot uses AI. Check for mistakes.
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.
is it possible to change the file name to snake-case?
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.
Same here, it would be great to use the same file name casing everywhere.
} | ||
|
||
then<TResult1 = T, TResult2 = never>( | ||
onfulfilled?: ((value: T) => TResult1 | PromiseLike<TResult1>) | undefined | null, |
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.
nitpick: the name should be onFulfilled
. Same comment for onRejected
and onFinally
export type BaseURL = string; | ||
|
||
export interface ClientConfig { | ||
BASE: BaseURL; |
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.
wondering why the field names are in upper case?
} | ||
|
||
// Backwards/ergonomic alias used by generated services | ||
export type ApiRequestOptions = RequestOptions; |
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.
nitpick: wondering if we can rename RequestOption
to ApiRequestOption
so we don't need to declare another type
// eslint-disable-next-line @typescript-eslint/no-var-requires | ||
const JSONBigFactory = require("json-bigint"); | ||
|
||
export type IntDecoding = "safe" | "unsafe" | "mixed" | "bigint"; |
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.
I think this should be an enum with documents, similar to the existing sdk
enum IntDecoding {
/**
* All integers will be decoded as Numbers, meaning any values greater than
* Number.MAX_SAFE_INTEGER will lose precision.
*/
UNSAFE = "unsafe",
/**
* All integers will be decoded as Numbers, but if any values are greater than
* Number.MAX_SAFE_INTEGER an error will be thrown.
*/
SAFE = "safe",
/**
* Integers will be decoded as Numbers if they are less than or equal to
* Number.MAX_SAFE_INTEGER, otherwise they will be decoded as BigInts.
*/
MIXED = "mixed",
/**
* All integers will be decoded as BigInts.
*/
BIGINT = "bigint"
}
if (options.path) { | ||
for (const [key, value] of Object.entries(options.path)) { | ||
const raw = typeof value === "bigint" ? value.toString() : String(value); | ||
const replace = config.ENCODE_PATH ? config.ENCODE_PATH(raw) : encodeURIPath(raw); |
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.
nitpick: I think encoded
is a better name
@@ -0,0 +1,49 @@ | |||
import { describe } from "bun:test"; | |||
import algosdk from "algosdk"; |
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.
Should we add a TODO replace algosdk.kmd with our own kmd?
export function maybeDescribe(name: string, fn: (env: AlgodTestConfig) => void) { | ||
describe(name, () => fn(getAlgodEnv())); | ||
} |
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.
instead of this, wondering if we can implement
describe('some describe', () => {
const env = getAlgodEnv()
if('some test', () => {
// env should be defined here
})
})
|
||
// Create two transactions similar to Rust test | ||
// Transaction 1: Simple payment | ||
const txn1 = algosdk.makePaymentTxnWithSuggestedParamsFromObject({ |
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.
I think we should have TODO to replace algosdk with our logic.
@aorumbayev I noticed that the |
hi @aorumbayev, I tested the output from this branch and found some possible improvements
|
73124c1
to
bbe4668
Compare
Initial version of ts api jinja generator and resulting algod/indexer clients based off algo-fetch from awesomealgo.