PRB Feedback for TypeScript by PaulRBerg · Pull Request #203 · sablier-labs/solsab

PaulRBerg · 2025-07-24T20:02:53Z

PR

My feedback for the TypeScript part of the repo. Rust will follow suit.

Issues Closed

Closes Tighter coupling between base test modules and their set-up functions #208
Closes Cache Anchor build artifacts #207
Closes Disable compiling logs in CI #205
Closes Do not test our own errors in "createWithDurations" and "createWithTimestamps" #202
Closes Replace Prettier and ESlint setup with Biome #189
Closes Assertion helper for account existence #188
Closes Use already-defined error codes from the "target/idl" #187
Closes Use more declarative "expect(promise).rejects.toThrow" testing syntax #186
Closes Use up-to-date GitHub Action for installing Rust #183
Closes Type-safe program names #182
Closes Type-safe error management in tests #179
Closes Use a single big number library #178
Closes SyntaxError: Named export 'BN' not found #176
Closes Use BUSL license #175

PaulRBerg · 2025-07-25T08:24:53Z

fixing the conflicts now

- Add codegen error codes - Use namespaces for test defaults - Use helpers to convert to and from BN - Fixed versions for Solana and Anchor CLI - Add constants, e.g., ZERO - Move shared TypeScript logic to "lib" directory - Setup husky and lint-staged - Switch to BUSL license - Migrate from mocha to vitest - Use only bn.js for big numbers - Format files with biome - Major refactoring improvements

ci: skip build and tests if cache is identical ci: suppress annoying anchor build logs

andreivladbrg · 2025-07-25T16:52:28Z

@PaulRBerg in order to move faster with the review process, i'll leave questions sequentially

i hope it works for you

andreivladbrg

left a few more comments

btw, as always, you're very good with structural changes 🫡 - it's much better organized (i suppose the SDK work made you a pro at TS 😆)

closes #193

andreivladbrg

just had a look at the new .test files

they seem to be the same tests, just refactored to the context pattern + vitest - the removal of try/catch makes the code considerably more readable

gj again👌

PaulRBerg · 2025-07-27T17:32:36Z

Thanks @andreivladbrg for your feedback - glad you like the new structure!

style: indenting for even-better-toml

PaulRBerg · 2025-07-27T19:04:47Z

nice one with d076eb2, @andreivladbrg!

refactor: dry codegen script utils

PaulRBerg · 2025-07-27T19:12:43Z

Alright, we now also have auto-generated struct types. Can you plz review @andreivladbrg @IaroslavMazur?

andreivladbrg

left few more comments on the new codegen scripts

IaroslavMazur

Great stuff, @PaulRBerg! 👏 Appreciate the numerous optimisations and simplifications! 💪

Pushed some small changes in 2492604 - and leaving the rest of my feedback below.

IaroslavMazur · 2025-07-28T12:31:36Z

-import { PublicKey } from "@solana/web3.js";
+import { type PublicKey } from "@solana/web3.js";
+import type BN from "bn.js";
 import keccak256 from "keccak256";


Getting the following TS warning for keccak256, as well as BN:

Module '"/Users/iaros/Desktop/Projects/solsab/node_modules/keccak256/dist/keccak256"' can only be default-imported using the 'esModuleInterop' flagts(1259) keccak256.d.ts(5, 1): This module is declared with 'export =', and can only be used with a default import when using the 'esModuleInterop' flag.

We used to have "esModuleInterop": true in tsconfig.json, but it has been removed in this PR.

We're still using esModuleInterop because we're extending the shared TSConfig from the devkit.

I don't know why you get that warning. Do you get it in VSCode? If yes, can you confirm you are using the node_modules-installed TypeScript?

Otherwise, can you confirm you are using Node.js v23 or later?

Do you get it in VSCode?

Yes.

If yes, can you confirm you are using the node_modules-installed TypeScript?

Nope, it appears that I'm running a "global" TypeScript, as which tsc returns /Users/iaros/.nvm/versions/node/v23.8.0/bin/tsc.

This is weird, because SolSab's package.json contains "typescript": "5.8.3" inside devDependencies{} 🤔

refactor: remove unused types refactor: import types from anchor package test: rename params in assertion helpers

andreivladbrg

looks good to me, just one small comment

Co-authored-by: Iaroslav Mazur <iaroslav.mazur@proton.me>

PaulRBerg requested review from IaroslavMazur and andreivladbrg and removed request for andreivladbrg July 25, 2025 08:24

PaulRBerg force-pushed the prb-feedback branch from bcab120 to e1e7013 Compare July 25, 2025 08:40

PaulRBerg mentioned this pull request Jul 25, 2025

Use up-to-date GitHub Action for installing Rust #183

Closed

PaulRBerg force-pushed the prb-feedback branch 2 times, most recently from 817f39d to 20e8307 Compare July 25, 2025 09:33

PaulRBerg mentioned this pull request Jul 25, 2025

ci: various improvements #184

Closed

PaulRBerg force-pushed the prb-feedback branch 5 times, most recently from 3e94dc9 to f1636f7 Compare July 25, 2025 14:46

PaulRBerg force-pushed the prb-feedback branch from f1636f7 to 95f40e3 Compare July 25, 2025 15:06

ci: cache build artifacts

bdd1a90

ci: skip build and tests if cache is identical ci: suppress annoying anchor build logs

PaulRBerg mentioned this pull request Jul 25, 2025

Cache Anchor build artifacts #207

Closed

andreivladbrg reviewed Jul 25, 2025

View reviewed changes

Comment thread justfile Outdated

Comment thread .prettierignore

Comment thread CONTRIBUTING.md Outdated

Comment thread CONTRIBUTING.md Outdated

Comment thread LICENSE.md

Comment thread justfile Outdated

Comment thread justfile Outdated

PaulRBerg force-pushed the prb-feedback branch from 63c77bd to bd622ae Compare July 25, 2025 18:08

test: context class instead of loose base

90e05f5

PaulRBerg force-pushed the prb-feedback branch from bd622ae to 90e05f5 Compare July 25, 2025 18:12

andreivladbrg reviewed Jul 25, 2025

View reviewed changes

Comment thread CONTRIBUTING.md

andreivladbrg reviewed Jul 26, 2025

View reviewed changes

Comment thread lib/types.ts

Comment thread scripts/ts/codegen-errors.ts

Comment thread programs/merkle_instant/Cargo.toml

Comment thread tests/common/assertions.ts

Comment thread tests/common/context.ts

Comment thread tests/common/context.ts

Comment thread vitest.config.ts

chore: include reporters in vitest config

d076eb2

closes #193

andreivladbrg reviewed Jul 27, 2025

View reviewed changes

Comment thread tests/lockup/utils/defaults.ts

Comment thread tests/lockup/utils/types.ts

feat: codegen-structs

9867adc

style: indenting for even-better-toml

feat: integrate and use codegen-structs

4d1344f

refactor: dry codegen script utils

PaulRBerg and others added 3 commits July 28, 2025 11:25

chore: add build log

1443dd1

chore: allow unexpected_cfgs

378a06c

chore: nitpicks

6d87886

andreivladbrg reviewed Jul 28, 2025

View reviewed changes

refactor: naming & cleaning

2492604

IaroslavMazur requested changes Jul 28, 2025

View reviewed changes

andreivladbrg reviewed Jul 28, 2025

View reviewed changes

Comment thread scripts/ts/codegen-errors.ts Outdated

fix: address PR feedback

b763c35

refactor: remove unused types refactor: import types from anchor package test: rename params in assertion helpers

andreivladbrg approved these changes Jul 28, 2025

View reviewed changes

Comment thread scripts/ts/common/codegen-utils.ts

PaulRBerg and others added 5 commits July 28, 2025 20:47

perf: look only for BN and PublicKey

b537e15

docs: update license in "package.json"

030805c

Co-authored-by: Iaroslav Mazur <iaroslav.mazur@proton.me>

tests: namespaces in defaults.ts

242c3eb

scripts: refactor extractIdlField()

750af46

style: format

79e0e4e

andreivladbrg merged commit 407d9db into main Jul 28, 2025
1 check passed

andreivladbrg deleted the prb-feedback branch July 28, 2025 18:20

andreivladbrg mentioned this pull request Jul 28, 2025

awarning: unexpected cfg condition value: #180

Closed

Conversation

PaulRBerg commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR

Issues Closed

Uh oh!

PaulRBerg commented Jul 25, 2025

Uh oh!

andreivladbrg commented Jul 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andreivladbrg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andreivladbrg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PaulRBerg commented Jul 27, 2025

Uh oh!

PaulRBerg commented Jul 27, 2025

Uh oh!

PaulRBerg commented Jul 27, 2025

Uh oh!

andreivladbrg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IaroslavMazur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IaroslavMazur Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

PaulRBerg Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

IaroslavMazur Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andreivladbrg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

PaulRBerg commented Jul 24, 2025 •

edited

Loading