Conversation
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com>
|
|
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideIntroduce a typed precondition system for the relayer and API client, including runtime type guards and encoding helpers for protobuf RPC, while slightly adjusting the API client base import and request header typing. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- Several
isXPreconditionhelpers (e.g.isErc721OwnershipPrecondition,isErc721ApprovalPrecondition) don't guard againstnullvalues while others do (precondition &&), which can cause runtime errors onnullinputs; consider consistently checking for non-null objects before accessing properties. - The various
encode*Preconditionfunctions spread the input object and then settype: undefined, which still leaves atypekey present on the encoded payload; iftypeshould be omitted entirely from the nestedprecondition, consider using object rest or an explicit omit instead of assigningundefined. - In
SequenceAPIClient._fetch, theheadersobject is typed as{ [key: string]: any }even though the Fetch API expects string header values; tightening this to{ [key: string]: string }(or a more precise type) would avoid unintentionally passing non-string values intofetch.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several `isXPrecondition` helpers (e.g. `isErc721OwnershipPrecondition`, `isErc721ApprovalPrecondition`) don't guard against `null` values while others do (`precondition &&`), which can cause runtime errors on `null` inputs; consider consistently checking for non-null objects before accessing properties.
- The various `encode*Precondition` functions spread the input object and then set `type: undefined`, which still leaves a `type` key present on the encoded payload; if `type` should be omitted entirely from the nested `precondition`, consider using object rest or an explicit omit instead of assigning `undefined`.
- In `SequenceAPIClient._fetch`, the `headers` object is typed as `{ [key: string]: any }` even though the Fetch API expects string header values; tightening this to `{ [key: string]: string }` (or a more precise type) would avoid unintentionally passing non-string values into `fetch`.
## Individual Comments
### Comment 1
<location path="packages/services/api/src/index.ts" line_range="23" />
<code_context>
// automatically include jwt and access key auth header to requests
// if its been set on the api client
- const headers: Record<string, string> = {}
+ const headers: { [key: string]: any } = {}
const jwtAuth = this.jwtAuth
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Headers typing is overly permissive and loses the previous string-only constraint.
Changing from `Record<string, string>` to `{ [key: string]: any }` means header values are no longer guaranteed to be strings, which can cause unexpected coercions or subtle bugs when passing `headers` to `fetch`. If you need optional values, prefer a stricter type such as `Record<string, string | undefined>` or a more precise union instead of `any`.
Suggested implementation:
```typescript
// automatically include jwt and access key auth header to requests
// if its been set on the api client
const headers: Record<string, string | undefined> = {}
```
If elsewhere in this file you are assigning non-string values to `headers` (e.g. numbers, booleans, objects), those assignments should be updated to `.toString()` or otherwise converted to strings to comply with the stricter type. Also ensure `headers` is actually wired into the `fetch` call (e.g. via `init = { ...init, headers }`), if that was the original intent of this helper.
</issue_to_address>
### Comment 2
<location path="packages/services/api/src/index.ts" line_range="54-55" />
<code_context>
+}
+
+export function encodePrecondition(precondition: Precondition): proto.Precondition {
+ const { type, precondition: args } = encodeChainPrecondition(precondition)
+ delete args.chainId
+ return { type, chainID: encodeBigNumberish(precondition.chainId), precondition: args }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid mutating the `args` object returned from `encodeChainPrecondition`.
`delete args.chainId` mutates the object from `encodeChainPrecondition`, which may be reused or assumed immutable. Instead, destructure to omit `chainId`, e.g. `const { chainId: _ignored, ...rest } = args`, and use `rest` when building the proto precondition to keep this function side-effect-free.
</issue_to_address>
### Comment 3
<location path="packages/services/api/src/index.ts" line_range="68-71" />
<code_context>
+ }
+}
+
+function encodeBigNumberish<T extends ethers.BigNumberish | undefined>(
+ value: T
+): T extends ethers.BigNumberish ? string : undefined {
+ return value !== undefined ? ethers.toBigInt(value).toString() : (undefined as any)
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Generic return typing plus `(undefined as any)` is misleading and undermines type safety.
The conditional return type coupled with `undefined as any` lets callers assume types that don’t match runtime behavior. Prefer a simple `encodeBigNumberish(value: ethers.BigNumberish | undefined): string | undefined` to keep types aligned with the implementation, or use overloads if you truly need distinct call signatures.
Suggested implementation:
```typescript
function isBigNumberish(value: any): value is ethers.BigNumberish {
try {
ethers.toBigInt(value)
return true
} catch {
return false
}
}
function encodeBigNumberish(value: ethers.BigNumberish | undefined): string | undefined {
return value !== undefined ? ethers.toBigInt(value).toString() : undefined
}
```
```typescript
export * from './api.gen'
export type Precondition = { chainId: ethers.BigNumberish } & ChainPrecondition
export function isPrecondition(precondition: any): precondition is Precondition {
return (
typeof precondition === 'object' && precondition && isBigNumberish(precondition.chainId) && isChainPrecondition(precondition)
)
}
```
If there are call sites that relied on the generic conditional return type (e.g., expecting `string` when passing a non-`undefined` type argument), they may need minor type adjustments to accept `string | undefined` or, if necessary, explicit non-null assertions after appropriate runtime checks.
</issue_to_address>
### Comment 4
<location path="packages/relayer/src/precondition.ts" line_range="143-150" />
<code_context>
+ owned?: boolean
+}
+
+function isErc721OwnershipPrecondition(precondition: any): precondition is Erc721OwnershipPrecondition {
+ return (
+ typeof precondition === 'object' &&
+ precondition.type === 'erc721-ownership' &&
+ ethers.isAddress(precondition.address) &&
+ ethers.isAddress(precondition.token) &&
+ isBigNumberish(precondition.tokenId) &&
+ (precondition.owned === undefined || typeof precondition.owned === 'boolean')
+ )
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Inconsistent null/undefined guard compared to other `is*Precondition` functions may cause runtime errors.
Other `is*Precondition` predicates (e.g. `isNativeBalancePrecondition`, `isErc20BalancePrecondition`) guard with `precondition && typeof precondition === 'object'`. Here, passing `null` still satisfies `typeof precondition === 'object'`, so `precondition.type` will throw. Please add a truthiness check (`precondition &&`) here and in other similar predicates like `isErc721ApprovalPrecondition` for consistency and to avoid runtime errors.
</issue_to_address>
### Comment 5
<location path="packages/relayer/src/precondition.ts" line_range="67-68" />
<code_context>
+function encodeNativeBalancePrecondition(precondition: NativeBalancePrecondition): proto.Precondition {
+ return {
+ type: precondition.type,
+ precondition: {
+ ...precondition,
+ type: undefined,
+ min: encodeBigNumberish(precondition.min),
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Spreading `precondition` and then setting `type: undefined` leaves an unnecessary `type` field in the encoded object.
Across multiple encoders (native/erc20/erc1155), `precondition: { ...precondition, type: undefined, ... }` leaves a `type` key with value `undefined` in the serialized proto, which can conflict with schema expectations. Consider destructuring to drop `type` entirely, e.g. `const { type, ...rest } = precondition;` and then using `precondition: { ...rest, ... }` so the field is removed rather than set to `undefined`.
Suggested implementation:
```typescript
function encodeNativeBalancePrecondition(precondition: NativeBalancePrecondition): proto.Precondition {
const { type, ...rest } = precondition
return {
type,
precondition: {
...rest,
min: encodeBigNumberish(precondition.min),
max: encodeBigNumberish(precondition.max)
}
}
}
```
You mentioned similar encoder patterns for ERC20 and ERC1155 preconditions. Apply the same destructuring approach in their respective encode functions:
1. Destructure `type` out of the precondition argument: `const { type, ...rest } = precondition`.
2. Use `type` for the top-level `type` field.
3. Use `rest` for the nested `precondition` object, instead of spreading the original object and setting `type: undefined`.
This will consistently avoid emitting `type: undefined` in all encoded preconditions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Implement SequenceBatchService for token approval and calls. Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com>
Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com>
Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com>
This file contains an example of using SequenceBatchBuilder to approve and transfer ERC20 tokens on the BSC network. Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com>
Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Integrate typed precondition support into the relayer and API client, enabling encoding and validation of on-chain balance, approval, and ownership conditions.
New Features:
Enhancements: