Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors role resolution to use dynamic contract-method lookup with validation, exposes Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant TransferFn as transferOwnership()
participant Utils as getRoleString / helpers
participant Provider as Provider
participant Contract as DocumentStoreContract
Caller->>TransferFn: call(documentStoreAddr, newOwner, ...)
TransferFn->>Utils: getRoleString(documentStoreAddr, "DEFAULT_ADMIN_ROLE")
TransferFn->>Provider: getEthersContractFromProvider(addr, ABI)
TransferFn->>Contract: staticCall/grantRole pre-check
Contract-->>TransferFn: pre-check OK / error
TransferFn->>Contract: staticCall/revokeRole pre-check
Contract-->>TransferFn: pre-check OK / error
TransferFn->>Contract: send grantRole(tx)
Contract-->>TransferFn: grant tx (ContractTransaction)
TransferFn->>Contract: send revokeRole(tx)
Contract-->>TransferFn: revoke tx (ContractTransaction)
TransferFn-->>Caller: return { grantTransaction, revokeTransaction }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/document-store/transferOwnership.ts (1)
53-56: Truthiness checks ongrantTransaction/revokeTransactionare unreachable.Both
documentStoreGrantRoleanddocumentStoreRevokeRoleeither return a transaction object or throw an error — they never return a falsy value. If the underlying call fails, theawaiton lines 45/58 will throw before reaching these checks.These guards are dead code; removing them would reduce false reassurance.
Proposed simplification
const grantTransaction = await documentStoreGrantRole( documentStoreAddress, roleString, account, signer, options, ); - //check if the grant transaction is successful - if (!grantTransaction) { - //add custom error message not proceeding with the revoke transaction - throw new Error('Grant transaction failed, not proceeding with revoke transaction'); - } //call revoke function here const revokeTransaction = await documentStoreRevokeRole( documentStoreAddress, roleString, ownerAddress, signer, options, ); - //check if the revoke transaction is successful - if (!revokeTransaction) { - throw new Error('Revoke transaction failed'); - } return { grantTransaction, revokeTransaction };Also applies to: 66-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/document-store/transferOwnership.ts` around lines 53 - 56, The truthiness checks for grantTransaction and revokeTransaction in transferOwnership are dead code because documentStoreGrantRole and documentStoreRevokeRole either return a transaction or throw; remove the if (!grantTransaction) { ... } and if (!revokeTransaction) { ... } blocks around the variables grantTransaction and revokeTransaction, and instead wrap the await calls to documentStoreGrantRole/documentStoreRevokeRole (or the surrounding transferOwnership logic) in a try/catch that adds contextual error messages (e.g., "grant failed" / "revoke failed") before rethrowing, or simply let the original errors propagate for callers to handle.src/document-store/document-store-roles.ts (1)
33-40: Dynamic role lookup looks correct but allows calling any contract method, not just role constants.The validation on line 35 checks that
rolemaps to any function on the contract, not specifically a role-constant getter. A caller could pass e.g.'name'or'owner'and get a successful call. Consider an allowlist if you want to restrict to role methods only.This is low-risk since the call is read-only via a provider, but worth noting for defense-in-depth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/document-store/document-store-roles.ts` around lines 33 - 40, The dynamic role lookup in documentStore allows calling any contract function because it only checks typeof documentStore[role] === 'function'; tighten this by introducing an explicit allowlist of role getter names (e.g., 'DEFAULT_ADMIN_ROLE', 'ISSUER_ROLE', 'REVOKER_ROLE') and validate the incoming role against that list before invoking (documentStore[role]()); if role is not in the allowlist throw an error; keep the final call as the read-only provider call but only after allowlist validation to enforce role-only access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/document-store/document-store-roles.ts`:
- Line 43: rolesList currently contains shorthand names that don't match
getRoleString's expected contract method names, causing "Invalid role" when
passed to getRoleString; update the exported rolesList constant to use the exact
role identifiers used by the contract and getRoleString — namely
'DEFAULT_ADMIN_ROLE', 'ISSUER_ROLE', and 'REVOKER_ROLE' — so any external code
using rolesList will be compatible with getRoleString and contract method
lookups.
In `@src/document-store/transferOwnership.ts`:
- Around line 45-68: The current flow can leave both parties with the role if
grant succeeds but revoke fails; to fix, perform dry-run (callStatic) checks for
both grant and revoke before sending any on‑chain transaction: call the
static/preflight variant used by documentStoreGrantRole and
documentStoreRevokeRole (or directly use the contract's callStatic.grantRole and
callStatic.revokeRole with the same args: documentStoreAddress, roleString,
account, ownerAddress and signer/options) and abort with a clear error if either
static call would fail; only after both static checks pass, submit the real
grant (documentStoreGrantRole) and then the real revoke
(documentStoreRevokeRole), keeping the existing grantTransaction and
revokeTransaction checks.
---
Nitpick comments:
In `@src/document-store/document-store-roles.ts`:
- Around line 33-40: The dynamic role lookup in documentStore allows calling any
contract function because it only checks typeof documentStore[role] ===
'function'; tighten this by introducing an explicit allowlist of role getter
names (e.g., 'DEFAULT_ADMIN_ROLE', 'ISSUER_ROLE', 'REVOKER_ROLE') and validate
the incoming role against that list before invoking (documentStore[role]()); if
role is not in the allowlist throw an error; keep the final call as the
read-only provider call but only after allowlist validation to enforce role-only
access.
In `@src/document-store/transferOwnership.ts`:
- Around line 53-56: The truthiness checks for grantTransaction and
revokeTransaction in transferOwnership are dead code because
documentStoreGrantRole and documentStoreRevokeRole either return a transaction
or throw; remove the if (!grantTransaction) { ... } and if (!revokeTransaction)
{ ... } blocks around the variables grantTransaction and revokeTransaction, and
instead wrap the await calls to documentStoreGrantRole/documentStoreRevokeRole
(or the surrounding transferOwnership logic) in a try/catch that adds contextual
error messages (e.g., "grant failed" / "revoke failed") before rethrowing, or
simply let the original errors propagate for callers to handle.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/document-store/document-store-roles.tssrc/document-store/index.tssrc/document-store/transferOwnership.tssrc/index.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/document-store/document-store-roles.ts (1)
43-43:rolesListcorrectly updated to contract method names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/document-store/document-store-roles.ts` at line 43, The rolesList constant was updated to match the contract method names; keep export const rolesList = ['DEFAULT_ADMIN_ROLE', 'ISSUER_ROLE', 'REVOKER_ROLE'] as-is in document-store-roles.ts (ensure the symbol rolesList remains exported and named exactly so callers referencing rolesList or these role names match the smart contract methods).
🧹 Nitpick comments (1)
src/document-store/transferOwnership.ts (1)
58-86: Redundant interface checks: ABI detection runs 3× across this function and both helpers.From the
grant-role.tsandrevoke-role.tssnippets, bothdocumentStoreGrantRoleanddocumentStoreRevokeRoleindependently runcheckSupportsInterface(twice each) unlessisTransferableis explicitly passed inoptions. SinceisTransferableis already resolved here (line 65), pass it down to both helpers to avoid 4 additionaleth_callRPCs on every ownership transfer.Additionally, lines 60–69 run the two interface checks sequentially; parallelizing them shaves one round-trip:
♻️ Proposed improvements
- const isDocumentStore = await checkSupportsInterface( - documentStoreAddress, - supportInterfaceIds.IDocumentStore, - signer.provider, - ); - const isTransferableDocumentStore = await checkSupportsInterface( - documentStoreAddress, - supportInterfaceIds.ITransferableDocumentStore, - signer.provider, - ); + const [isDocumentStore, isTransferableDocumentStore] = await Promise.all([ + checkSupportsInterface(documentStoreAddress, supportInterfaceIds.IDocumentStore, signer.provider), + checkSupportsInterface(documentStoreAddress, supportInterfaceIds.ITransferableDocumentStore, signer.provider), + ]);Then thread the resolved flag into the helpers:
const grantTransaction = await documentStoreGrantRole( documentStoreAddress, roleString, account, signer, - options, + { ...options, isTransferable: isTransferableDocumentStore }, ); // …and similarly for documentStoreRevokeRole🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/document-store/transferOwnership.ts` around lines 58 - 86, The code runs checkSupportsInterface twice sequentially and duplicates those checks in downstream helpers; run both interface checks in parallel (e.g. Promise.all) when resolving isDocumentStore/isTransferableDocumentStore in transferOwnership.ts (the existing calls to checkSupportsInterface for supportInterfaceIds.IDocumentStore and supportInterfaceIds.ITransferableDocumentStore), derive the documentStoreAbi as now, then thread the resolved boolean (isTransferableDocumentStore) into the helper calls documentStoreGrantRole and documentStoreRevokeRole so those functions no longer re-run checkSupportsInterface; update those helper signatures to accept an optional isTransferable flag and skip their internal checks when that flag is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/document-store/transferOwnership.ts`:
- Around line 99-109: The current brittle classification in transferOwnership.ts
(inside the catch handling around the callStatic pre-check) relies only on
substring matches of e.message; change it to first inspect structured ethers
error fields (e.reason, e.errorName, e.code, e.data or e.error?.name) and use
those values to classify the error into the existing buckets (unauthorized /
already-has-role / simulation-failed), falling back to the message-only
heuristic only if structured fields are absent; update the thrown Error to
include the discovered structured fields (errorName/reason/data) so callers see
actionable context while preserving the existing fallback text.
- Around line 132-158: Update the misleading comment to state that pre-checks
are performed twice (once here and again inside
documentStoreGrantRole/documentStoreRevokeRole) or indicate that an opt-out flag
should be threaded through if you choose to avoid double-checks; then remove the
dead null-guards around grantTransaction and revokeTransaction (the checks using
!grantTransaction / !revokeTransaction) because documentStoreGrantRole and
documentStoreRevokeRole are typed to throw on failure and always return a
ContractTransaction, so simply await the transactions and let exceptions
propagate or handle them via try/catch if needed; reference the variables
grantTransaction, revokeTransaction and the helper functions
documentStoreGrantRole/documentStoreRevokeRole when making these changes.
---
Duplicate comments:
In `@src/document-store/document-store-roles.ts`:
- Line 43: The rolesList constant was updated to match the contract method
names; keep export const rolesList = ['DEFAULT_ADMIN_ROLE', 'ISSUER_ROLE',
'REVOKER_ROLE'] as-is in document-store-roles.ts (ensure the symbol rolesList
remains exported and named exactly so callers referencing rolesList or these
role names match the smart contract methods).
---
Nitpick comments:
In `@src/document-store/transferOwnership.ts`:
- Around line 58-86: The code runs checkSupportsInterface twice sequentially and
duplicates those checks in downstream helpers; run both interface checks in
parallel (e.g. Promise.all) when resolving
isDocumentStore/isTransferableDocumentStore in transferOwnership.ts (the
existing calls to checkSupportsInterface for supportInterfaceIds.IDocumentStore
and supportInterfaceIds.ITransferableDocumentStore), derive the documentStoreAbi
as now, then thread the resolved boolean (isTransferableDocumentStore) into the
helper calls documentStoreGrantRole and documentStoreRevokeRole so those
functions no longer re-run checkSupportsInterface; update those helper
signatures to accept an optional isTransferable flag and skip their internal
checks when that flag is provided.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/document-store/transferOwnership.ts (2)
125-139: Remove unreachable null-guards after awaited helper calls.The
if (!grantTransaction)andif (!revokeTransaction)branches look redundant if helper contracts are typed to return a transaction or throw. Keeping them adds dead branches and misleading failure paths.♻️ Proposed cleanup
const grantTransaction = await documentStoreGrantRole( @@ ); - - if (!grantTransaction) { - throw new Error('Grant transaction failed, not proceeding with revoke transaction'); - } const revokeTransaction = await documentStoreRevokeRole( @@ ); - - if (!revokeTransaction) { - throw new Error('Revoke transaction failed'); - }#!/bin/bash set -euo pipefail # Verify helper return types and ensure they are non-nullable transaction responses fd 'grant-role.ts|revoke-role.ts|transferOwnership.ts' src/document-store -t f rg -n -C3 'export const documentStoreGrantRole|export const documentStoreRevokeRole|Promise<|ContractTransaction|ContractTransactionResponse|\bnull\b|\bundefined\b' src/document-store🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/document-store/transferOwnership.ts` around lines 125 - 139, The null-check branches after awaiting documentStoreGrantRole and documentStoreRevokeRole are redundant; verify that documentStoreGrantRole and documentStoreRevokeRole are typed to return a non-null ContractTransaction (or throw on failure) and remove the if (!grantTransaction) and if (!revokeTransaction) guards and their throws in transferOwnership.ts so errors propagate naturally from the awaited helper calls; keep the awaits (assign to grantTransaction/revokeTransaction if needed) but eliminate the dead conditional branches to simplify control flow.
99-114:⚠️ Potential issue | 🟡 MinorPreserve revert context instead of throwing generic pre-check errors.
Both catch blocks discard actionable revert details by rethrowing fixed strings. This makes operational debugging harder when pre-checks fail.
♻️ Proposed improvement
} catch (e) { - console.error('callStatic failed:', e); - throw new Error('Pre-check (callStatic) for grant-role failed'); + const details = e instanceof Error ? e.message : String(e); + throw new Error(`Pre-check (callStatic) for grant-role failed: ${details}`); } @@ } catch (e) { - console.error('callStatic failed:', e); - throw new Error('Pre-check (callStatic) for revoke-role failed'); + const details = e instanceof Error ? e.message : String(e); + throw new Error(`Pre-check (callStatic) for revoke-role failed: ${details}`); }#!/bin/bash set -euo pipefail # Verify whether the codebase already has a structured ethers error parsing pattern rg -n -C3 'errorName|reason|shortMessage|callStatic failed|Pre-check \(callStatic\)' src🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/document-store/transferOwnership.ts` around lines 99 - 114, The catch blocks for the callStatic pre-checks (the grant-role and revoke-role handlers around the calls to (documentStoreContract as ContractV6).revokeRole.staticCall and (documentStoreContract as ContractV5).callStatic.revokeRole) currently throw generic Errors and lose the revert context; change them to preserve and surface the original revert details by including the captured error in the thrown Error (e.g. include error.reason / error.error?.message / error.shortMessage or use new Error('Pre-check (callStatic) for revoke-role failed: ' + String(e)) or throw new Error('...', { cause: e })), so the thrown error or log contains the original ethers revert message/stack for easier debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/document-store/transferOwnership.ts`:
- Around line 125-139: The null-check branches after awaiting
documentStoreGrantRole and documentStoreRevokeRole are redundant; verify that
documentStoreGrantRole and documentStoreRevokeRole are typed to return a
non-null ContractTransaction (or throw on failure) and remove the if
(!grantTransaction) and if (!revokeTransaction) guards and their throws in
transferOwnership.ts so errors propagate naturally from the awaited helper
calls; keep the awaits (assign to grantTransaction/revokeTransaction if needed)
but eliminate the dead conditional branches to simplify control flow.
- Around line 99-114: The catch blocks for the callStatic pre-checks (the
grant-role and revoke-role handlers around the calls to (documentStoreContract
as ContractV6).revokeRole.staticCall and (documentStoreContract as
ContractV5).callStatic.revokeRole) currently throw generic Errors and lose the
revert context; change them to preserve and surface the original revert details
by including the captured error in the thrown Error (e.g. include error.reason /
error.error?.message / error.shortMessage or use new Error('Pre-check
(callStatic) for revoke-role failed: ' + String(e)) or throw new Error('...', {
cause: e })), so the thrown error or log contains the original ethers revert
message/stack for easier debugging.
rongquan1
approved these changes
Feb 25, 2026
nghaninn
pushed a commit
that referenced
this pull request
Feb 25, 2026
## [2.9.1](v2.9.0...v2.9.1) (2026-02-25) ### Bug Fixes * update document store functions ([#132](#132)) ([b87af71](b87af71))
|
🎉 This PR is included in version 2.9.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
Fixes for document store function updates.
Types and tests fixes
Summary by CodeRabbit
New Features
Improvements