feat: Enterprise workflows — shared multisig, approvals, team management, price triggers (#301, #315, #316, #317)#322
Merged
KevinMB0220 merged 2 commits intoJun 30, 2026
Conversation
…ice triggers Implements roadmap items Galaxy-KJ#301, Galaxy-KJ#315, Galaxy-KJ#316, and Galaxy-KJ#317 with shared wallet service, approval API routes, team management UI, and oracle price triggers. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds multi-sig approval REST services, an organization team management API and frontend panel, and a price-based automation trigger with tests and package exports. ChangesMulti-sig Approval Flow
Organization Team Management
PriceTrigger Automation Module
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: one or more packages not found in the registry. 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: 10
🧹 Nitpick comments (2)
packages/api/rest/src/routes/approvals/approvals.routes.test.ts (1)
69-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a smoke test for
GET /:proposalId.This suite covers propose/approve but never exercises the new read endpoint. A simple propose→get round-trip would pin the 200 path and the 404 mapping for unknown proposal IDs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/rest/src/routes/approvals/approvals.routes.test.ts` around lines 69 - 167, Add a smoke test for the new GET /:proposalId read path in approvals.routes.test.ts. Reuse the existing propose flow in the approvals routes tests, then fetch the created proposal through the GET endpoint and assert a 200 with the expected proposal data. Also add a negative case for an unknown proposalId to verify the 404 mapping in the same route handler.packages/core/automation/src/test/price-trigger.test.ts (1)
19-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit
'above'comparison test.The suite currently proves the
'below'branch, but not the real'above'comparison path inevaluate(). A dedicated'above'case would close that gap and better match the acceptance criteria for both conditions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/automation/src/test/price-trigger.test.ts` around lines 19 - 106, The test suite for trigger.evaluate covers the 'below' path and an unavailable-prices case, but it does not explicitly verify the 'above' comparison branch. Add a dedicated test alongside the existing trigger.evaluate cases that mocks oracle.getAggregatedPrices with a pair price above the threshold and asserts the result is true, using the same assetIn/assetOut, condition, and threshold pattern so the 'above' logic is directly exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/rest/src/routes/approvals/index.ts`:
- Around line 19-25: The approvals route currently forwards multisigConfig from
req.body without validation, which can allow malformed wallet-registration data
to reach proposal creation; update the handler that destructures req.body and
the related approval path to validate multisigConfig before auto-registering a
wallet. Require it to be an object with the expected threshold and signer
fields, and return a 400 from the approval endpoint when the config is missing
or invalid instead of letting downstream logic fail or persist a bad
registration.
- Around line 15-17: The approvals routes currently authenticate the caller but
do not enforce workspace membership or signer authorization, so the handler flow
in router.use(...) and the ApprovalService calls need to carry the authenticated
actor context. Update the /propose, read, and submit paths to read req.user and
req.permissions, pass that caller/org context into ApprovalService methods, and
reject requests when the caller is not in the target workspace or not in the
allowed signer set. Use the route handlers in the approvals index and the
ApprovalService entrypoints as the main places to thread and validate this
authorization.
In `@packages/api/rest/src/routes/teams/index.ts`:
- Around line 16-90: The team routes in router.use/authenticate and the handlers
for listMembers, inviteMember, updateMemberRole, and listActivity only
authenticate the user but never check organization membership or authorization
for the target :organizationId/member. Add an organization-level authorization
guard in these route handlers (or a shared middleware) that verifies the caller
belongs to req.params.organizationId and has permission to view members/activity
or manage roles before calling teamService.listMembers,
teamService.inviteMember, teamService.updateMemberRole, or
teamService.listActivity. Ensure POST /:organizationId/invite and PUT
/members/:memberId/role are restricted to users allowed to invite/change roles,
and return an appropriate forbidden/unauthorized response when checks fail.
- Around line 23-57: The teams invite route currently lets expected invite
failures from teamService.inviteMember() fall through to the global error
handler, which can turn duplicate-member and blank-email cases into 500s. Update
the router.post('/:organizationId/invite') handler to catch the specific invite
errors thrown by inviteMember and return appropriate 4xx responses instead of
calling next(error); keep the existing validation behavior and use the
inviteMember, VALID_ROLES, and TeamRole logic to locate the handling.
In `@packages/api/rest/src/services/approval-service.ts`:
- Around line 59-74: The fallback multisig config in propose() is unsatisfiable
because DEFAULT_MULTISIG_CONFIG keeps medium/high thresholds while the
auto-generated signer list only adds creatorPublicKey with weight 1. Update the
proposal registration logic in approval-service.ts so that when multisigConfig
is omitted, the effective config is internally consistent by either lowering the
default thresholds for the fallback path or constructing enough default
signers/weights to satisfy the required thresholds; use propose(),
DEFAULT_MULTISIG_CONFIG, and sharedWallets.registerWallet as the key places to
adjust.
- Around line 95-123: `ApprovalService.approve` is marking proposals as EXECUTED
even though `broadcastProposal` only generates a mock hash and does not submit
anything. Update `approve()` to avoid returning execution success from the mock
path, and keep the proposal in a non-executed state until real broadcast logic
exists; also make sure `getProposal()` and `broadcastProposal()` remain
consistent with that interim behavior.
In `@packages/api/rest/src/services/team-service.ts`:
- Around line 43-79: The activity logging in TeamService is using the target
member’s email as the actor, which makes the audit trail incorrect. Update
inviteMember() and updateMemberRole() to accept the authenticated actor identity
as an additional argument, then pass that actor through to appendActivity()
instead of input.email/member.email. Keep the existing member lookup and role
update logic intact, and use the same actor value wherever these methods record
workspace activity.
In `@packages/core/automation/src/triggers/price-trigger.ts`:
- Around line 23-25: The threshold validation in price-trigger’s numeric parsing
currently lets blank strings like '' and ' ' coerce to 0, so reject empty or
whitespace-only values before calling Number(config.threshold). Update the
validation in the trigger’s threshold handling to trim and check the raw
config.threshold first, then only coerce nonblank input and keep the existing
finite-number guard. Make sure the logic around the threshold setup in
price-trigger.ts still throws for invalid config and only accepts real numeric
strings.
In `@packages/core/wallet/src/shared-wallet.service.ts`:
- Around line 47-63: Reject duplicate wallet registrations in registerWallet
rather than letting this.wallets.set overwrite an existing entry keyed by
walletAddress. Add a guard before creating the new MultiSigWallet so repeated
registrations for the same walletAddress are ignored or fail explicitly, and
keep the current organizationId, signerWeights, and wallet state intact. If
updates are intended, route them through a separate explicit update path instead
of reusing registerWallet.
In `@packages/frontend/src/panels/team-management.ts`:
- Around line 98-138: The team management rendering in renderMembers and
renderActivity is injecting API-provided strings through innerHTML, which
creates a stored XSS path via member.email, entry.actorEmail, and entry.action.
Rework these DOM updates to build the table rows and activity items with DOM
APIs and textContent (or sanitize the values before insertion), and keep the
existing renderMembers/renderActivity flow and element selectors intact.
---
Nitpick comments:
In `@packages/api/rest/src/routes/approvals/approvals.routes.test.ts`:
- Around line 69-167: Add a smoke test for the new GET /:proposalId read path in
approvals.routes.test.ts. Reuse the existing propose flow in the approvals
routes tests, then fetch the created proposal through the GET endpoint and
assert a 200 with the expected proposal data. Also add a negative case for an
unknown proposalId to verify the 404 mapping in the same route handler.
In `@packages/core/automation/src/test/price-trigger.test.ts`:
- Around line 19-106: The test suite for trigger.evaluate covers the 'below'
path and an unavailable-prices case, but it does not explicitly verify the
'above' comparison branch. Add a dedicated test alongside the existing
trigger.evaluate cases that mocks oracle.getAggregatedPrices with a pair price
above the threshold and asserts the result is true, using the same
assetIn/assetOut, condition, and threshold pattern so the 'above' logic is
directly exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6baf35eb-30dd-43fe-9cbc-064f45d2f012
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
packages/api/rest/jest.config.jspackages/api/rest/package.jsonpackages/api/rest/src/index.tspackages/api/rest/src/routes/approvals/approvals.routes.test.tspackages/api/rest/src/routes/approvals/index.tspackages/api/rest/src/routes/teams/index.tspackages/api/rest/src/services/approval-service.tspackages/api/rest/src/services/team-service.tspackages/core/automation/index.tspackages/core/automation/src/test/price-trigger.test.tspackages/core/automation/src/triggers/price-trigger.tspackages/core/wallet/src/index.tspackages/core/wallet/src/shared-wallet.service.tspackages/core/wallet/src/tests/shared-wallet.service.test.tspackages/frontend/src/app.tspackages/frontend/src/panels/team-management.tspackages/frontend/src/services/team-management.client.tspackages/frontend/src/tests/team-management.panel.test.ts
Comment on lines
+15
to
+17
| router.use(authenticate(), auditRequest()); | ||
|
|
||
| router.post('/propose', async (req: Request, res: Response, next: NextFunction) => { |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Enforce workspace and signer authorization in this flow.
authenticate() proves who called the API, but these handlers never use req.user/req.permissions or pass caller context into ApprovalService. That means an authenticated user can propose, read, or submit approvals for another organization's proposal as long as they know the IDs and can supply a valid signature. Thread the authenticated actor and organization membership into the service contract and reject callers outside the workspace or allowed signer set.
Also applies to: 67-75, 81-167
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/rest/src/routes/approvals/index.ts` around lines 15 - 17, The
approvals routes currently authenticate the caller but do not enforce workspace
membership or signer authorization, so the handler flow in router.use(...) and
the ApprovalService calls need to carry the authenticated actor context. Update
the /propose, read, and submit paths to read req.user and req.permissions, pass
that caller/org context into ApprovalService methods, and reject requests when
the caller is not in the target workspace or not in the allowed signer set. Use
the route handlers in the approvals index and the ApprovalService entrypoints as
the main places to thread and validate this authorization.
Comment on lines
+19
to
+25
| const { | ||
| organizationId, | ||
| walletAddress, | ||
| transactionXdr, | ||
| creatorPublicKey, | ||
| multisigConfig, | ||
| } = req.body ?? {}; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate multisigConfig before auto-registering a wallet.
This route forwards multisigConfig unchecked even though proposal creation is also the wallet-registration path. Missing or malformed signer/threshold config will surface as a downstream 500 or persist a bad registration instead of returning a 400. At minimum require an object with the expected threshold and signer fields here.
Also applies to: 67-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/rest/src/routes/approvals/index.ts` around lines 19 - 25, The
approvals route currently forwards multisigConfig from req.body without
validation, which can allow malformed wallet-registration data to reach proposal
creation; update the handler that destructures req.body and the related approval
path to validate multisigConfig before auto-registering a wallet. Require it to
be an object with the expected threshold and signer fields, and return a 400
from the approval endpoint when the config is missing or invalid instead of
letting downstream logic fail or persist a bad registration.
Comment on lines
+16
to
+90
| router.use(authenticate(), auditRequest()); | ||
|
|
||
| router.get('/:organizationId/members', (req: Request, res: Response) => { | ||
| const members = teamService.listMembers(req.params.organizationId); | ||
| return res.status(200).json({ members }); | ||
| }); | ||
|
|
||
| router.post('/:organizationId/invite', (req: Request, res: Response, next: NextFunction) => { | ||
| try { | ||
| const { email, role } = req.body ?? {}; | ||
|
|
||
| if (!email || typeof email !== 'string') { | ||
| return res.status(400).json({ | ||
| error: { | ||
| code: 'VALIDATION_ERROR', | ||
| message: 'email is required', | ||
| details: {}, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| const normalizedRole = (role ?? 'member') as TeamRole; | ||
| if (!VALID_ROLES.includes(normalizedRole)) { | ||
| return res.status(400).json({ | ||
| error: { | ||
| code: 'VALIDATION_ERROR', | ||
| message: 'role must be admin, member, or viewer', | ||
| details: {}, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| const member = teamService.inviteMember({ | ||
| organizationId: req.params.organizationId, | ||
| email, | ||
| role: normalizedRole, | ||
| }); | ||
|
|
||
| return res.status(201).json({ member }); | ||
| } catch (error) { | ||
| next(error); | ||
| } | ||
| }); | ||
|
|
||
| router.put('/members/:memberId/role', (req: Request, res: Response, next: NextFunction) => { | ||
| try { | ||
| const { role } = req.body ?? {}; | ||
| if (!role || !VALID_ROLES.includes(role)) { | ||
| return res.status(400).json({ | ||
| error: { | ||
| code: 'VALIDATION_ERROR', | ||
| message: 'role must be admin, member, or viewer', | ||
| details: {}, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| const member = teamService.updateMemberRole(req.params.memberId, role); | ||
| return res.status(200).json({ member }); | ||
| } catch (error) { | ||
| if (error instanceof Error && error.message.includes('not found')) { | ||
| return res.status(404).json({ | ||
| error: { | ||
| code: 'MEMBER_NOT_FOUND', | ||
| message: error.message, | ||
| details: {}, | ||
| }, | ||
| }); | ||
| } | ||
| next(error); | ||
| } | ||
| }); | ||
|
|
||
| router.get('/:organizationId/activity', (req: Request, res: Response) => { | ||
| const activity = teamService.listActivity(req.params.organizationId); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Enforce organization-level authz on these endpoints.
These handlers only authenticate the caller; they never verify that the caller belongs to :organizationId or is allowed to invite admins / change roles. As written, any authenticated user can enumerate another workspace's members/activity and escalate privileges through POST /:organizationId/invite or PUT /members/:memberId/role.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/rest/src/routes/teams/index.ts` around lines 16 - 90, The team
routes in router.use/authenticate and the handlers for listMembers,
inviteMember, updateMemberRole, and listActivity only authenticate the user but
never check organization membership or authorization for the target
:organizationId/member. Add an organization-level authorization guard in these
route handlers (or a shared middleware) that verifies the caller belongs to
req.params.organizationId and has permission to view members/activity or manage
roles before calling teamService.listMembers, teamService.inviteMember,
teamService.updateMemberRole, or teamService.listActivity. Ensure POST
/:organizationId/invite and PUT /members/:memberId/role are restricted to users
allowed to invite/change roles, and return an appropriate forbidden/unauthorized
response when checks fail.
Comment on lines
+23
to
+57
| router.post('/:organizationId/invite', (req: Request, res: Response, next: NextFunction) => { | ||
| try { | ||
| const { email, role } = req.body ?? {}; | ||
|
|
||
| if (!email || typeof email !== 'string') { | ||
| return res.status(400).json({ | ||
| error: { | ||
| code: 'VALIDATION_ERROR', | ||
| message: 'email is required', | ||
| details: {}, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| const normalizedRole = (role ?? 'member') as TeamRole; | ||
| if (!VALID_ROLES.includes(normalizedRole)) { | ||
| return res.status(400).json({ | ||
| error: { | ||
| code: 'VALIDATION_ERROR', | ||
| message: 'role must be admin, member, or viewer', | ||
| details: {}, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| const member = teamService.inviteMember({ | ||
| organizationId: req.params.organizationId, | ||
| email, | ||
| role: normalizedRole, | ||
| }); | ||
|
|
||
| return res.status(201).json({ member }); | ||
| } catch (error) { | ||
| next(error); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return 4xx for expected invite failures.
TeamService.inviteMember() throws for duplicate members and blank-after-trim emails, but this route forwards both to the global error handler. That turns normal user mistakes into 500s instead of a conflict/validation response.
Proposed fix
} catch (error) {
+ if (error instanceof Error) {
+ if (error.message === 'Member already exists in organization') {
+ return res.status(409).json({
+ error: {
+ code: 'MEMBER_ALREADY_EXISTS',
+ message: error.message,
+ details: {},
+ },
+ });
+ }
+
+ if (error.message === 'email is required') {
+ return res.status(400).json({
+ error: {
+ code: 'VALIDATION_ERROR',
+ message: error.message,
+ details: {},
+ },
+ });
+ }
+ }
next(error);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| router.post('/:organizationId/invite', (req: Request, res: Response, next: NextFunction) => { | |
| try { | |
| const { email, role } = req.body ?? {}; | |
| if (!email || typeof email !== 'string') { | |
| return res.status(400).json({ | |
| error: { | |
| code: 'VALIDATION_ERROR', | |
| message: 'email is required', | |
| details: {}, | |
| }, | |
| }); | |
| } | |
| const normalizedRole = (role ?? 'member') as TeamRole; | |
| if (!VALID_ROLES.includes(normalizedRole)) { | |
| return res.status(400).json({ | |
| error: { | |
| code: 'VALIDATION_ERROR', | |
| message: 'role must be admin, member, or viewer', | |
| details: {}, | |
| }, | |
| }); | |
| } | |
| const member = teamService.inviteMember({ | |
| organizationId: req.params.organizationId, | |
| email, | |
| role: normalizedRole, | |
| }); | |
| return res.status(201).json({ member }); | |
| } catch (error) { | |
| next(error); | |
| } | |
| router.post('/:organizationId/invite', (req: Request, res: Response, next: NextFunction) => { | |
| try { | |
| const { email, role } = req.body ?? {}; | |
| if (!email || typeof email !== 'string') { | |
| return res.status(400).json({ | |
| error: { | |
| code: 'VALIDATION_ERROR', | |
| message: 'email is required', | |
| details: {}, | |
| }, | |
| }); | |
| } | |
| const normalizedRole = (role ?? 'member') as TeamRole; | |
| if (!VALID_ROLES.includes(normalizedRole)) { | |
| return res.status(400).json({ | |
| error: { | |
| code: 'VALIDATION_ERROR', | |
| message: 'role must be admin, member, or viewer', | |
| details: {}, | |
| }, | |
| }); | |
| } | |
| const member = teamService.inviteMember({ | |
| organizationId: req.params.organizationId, | |
| email, | |
| role: normalizedRole, | |
| }); | |
| return res.status(201).json({ member }); | |
| } catch (error) { | |
| if (error instanceof Error) { | |
| if (error.message === 'Member already exists in organization') { | |
| return res.status(409).json({ | |
| error: { | |
| code: 'MEMBER_ALREADY_EXISTS', | |
| message: error.message, | |
| details: {}, | |
| }, | |
| }); | |
| } | |
| if (error.message === 'email is required') { | |
| return res.status(400).json({ | |
| error: { | |
| code: 'VALIDATION_ERROR', | |
| message: error.message, | |
| details: {}, | |
| }, | |
| }); | |
| } | |
| } | |
| next(error); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/rest/src/routes/teams/index.ts` around lines 23 - 57, The teams
invite route currently lets expected invite failures from
teamService.inviteMember() fall through to the global error handler, which can
turn duplicate-member and blank-email cases into 500s. Update the
router.post('/:organizationId/invite') handler to catch the specific invite
errors thrown by inviteMember and return appropriate 4xx responses instead of
calling next(error); keep the existing validation behavior and use the
inviteMember, VALID_ROLES, and TeamRole logic to locate the handling.
Comment on lines
+59
to
+74
| propose(input: ProposeApprovalInput): Promise<string> { | ||
| const config = input.multisigConfig ?? DEFAULT_MULTISIG_CONFIG; | ||
| const existing = this.sharedWallets.getOrganizationWallets(input.organizationId); | ||
|
|
||
| if (!existing.includes(input.walletAddress)) { | ||
| this.sharedWallets.registerWallet({ | ||
| organizationId: input.organizationId, | ||
| walletAddress: input.walletAddress, | ||
| config: { | ||
| ...config, | ||
| signers: config.signers.length | ||
| ? config.signers | ||
| : [{ publicKey: input.creatorPublicKey, weight: 1 }], | ||
| }, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the fallback multisig config satisfiable.
When multisigConfig is omitted, the service registers only the creator with weight 1, but the default threshold requires medium: 2 and high: 3. Those proposals can never reach readiness for medium/high-threshold transactions.
Proposed validation
propose(input: ProposeApprovalInput): Promise<string> {
const config = input.multisigConfig ?? DEFAULT_MULTISIG_CONFIG;
+ const signers = config.signers.length
+ ? config.signers
+ : [{ publicKey: input.creatorPublicKey, weight: 1 }];
+ const totalWeight = signers.reduce((sum, signer) => sum + signer.weight, 0);
+ const maxRequiredWeight = Math.max(
+ config.threshold.low,
+ config.threshold.medium,
+ config.threshold.high
+ );
+
+ if (totalWeight < maxRequiredWeight) {
+ throw new Error('Multi-sig signer weights cannot satisfy configured thresholds');
+ }
+
const existing = this.sharedWallets.getOrganizationWallets(input.organizationId);
if (!existing.includes(input.walletAddress)) {
this.sharedWallets.registerWallet({
organizationId: input.organizationId,
walletAddress: input.walletAddress,
config: {
...config,
- signers: config.signers.length
- ? config.signers
- : [{ publicKey: input.creatorPublicKey, weight: 1 }],
+ signers,
},
});
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/rest/src/services/approval-service.ts` around lines 59 - 74, The
fallback multisig config in propose() is unsatisfiable because
DEFAULT_MULTISIG_CONFIG keeps medium/high thresholds while the auto-generated
signer list only adds creatorPublicKey with weight 1. Update the proposal
registration logic in approval-service.ts so that when multisigConfig is
omitted, the effective config is internally consistent by either lowering the
default thresholds for the fallback path or constructing enough default
signers/weights to satisfy the required thresholds; use propose(),
DEFAULT_MULTISIG_CONFIG, and sharedWallets.registerWallet as the key places to
adjust.
Comment on lines
+95
to
+123
| if (ready) { | ||
| const hash = await this.broadcastProposal(input.proposalId); | ||
| return { | ||
| ...proposal, | ||
| status: ProposalStatus.EXECUTED, | ||
| readyToExecute: false, | ||
| executionHash: hash, | ||
| }; | ||
| } | ||
|
|
||
| return proposal; | ||
| } | ||
|
|
||
| getProposal(proposalId: string): ApprovalProposalView | null { | ||
| const proposal = this.sharedWallets.getProposal(proposalId); | ||
| if (!proposal) { | ||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| ...proposal, | ||
| executionHash: this.executionHashes.get(proposalId), | ||
| }; | ||
| } | ||
|
|
||
| private async broadcastProposal(proposalId: string): Promise<string> { | ||
| const hash = `mock-broadcast-${proposalId.slice(0, 8)}`; | ||
| this.executionHashes.set(proposalId, hash); | ||
| return hash; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Don’t report execution after a mock broadcast.
broadcastProposal only creates a mock hash, but approve() returns EXECUTED. The transaction is not actually submitted, and later getProposal() can still read the underlying proposal as ready-to-execute.
Safer interim behavior until real broadcast is wired
private async broadcastProposal(proposalId: string): Promise<string> {
- const hash = `mock-broadcast-${proposalId.slice(0, 8)}`;
- this.executionHashes.set(proposalId, hash);
- return hash;
+ throw new Error(`Broadcast for proposal ${proposalId} is not implemented`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (ready) { | |
| const hash = await this.broadcastProposal(input.proposalId); | |
| return { | |
| ...proposal, | |
| status: ProposalStatus.EXECUTED, | |
| readyToExecute: false, | |
| executionHash: hash, | |
| }; | |
| } | |
| return proposal; | |
| } | |
| getProposal(proposalId: string): ApprovalProposalView | null { | |
| const proposal = this.sharedWallets.getProposal(proposalId); | |
| if (!proposal) { | |
| return null; | |
| } | |
| return { | |
| ...proposal, | |
| executionHash: this.executionHashes.get(proposalId), | |
| }; | |
| } | |
| private async broadcastProposal(proposalId: string): Promise<string> { | |
| const hash = `mock-broadcast-${proposalId.slice(0, 8)}`; | |
| this.executionHashes.set(proposalId, hash); | |
| return hash; | |
| if (ready) { | |
| const hash = await this.broadcastProposal(input.proposalId); | |
| return { | |
| ...proposal, | |
| status: ProposalStatus.EXECUTED, | |
| readyToExecute: false, | |
| executionHash: hash, | |
| }; | |
| } | |
| return proposal; | |
| } | |
| getProposal(proposalId: string): ApprovalProposalView | null { | |
| const proposal = this.sharedWallets.getProposal(proposalId); | |
| if (!proposal) { | |
| return null; | |
| } | |
| return { | |
| ...proposal, | |
| executionHash: this.executionHashes.get(proposalId), | |
| }; | |
| } | |
| private async broadcastProposal(proposalId: string): Promise<string> { | |
| throw new Error(`Broadcast for proposal ${proposalId} is not implemented`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/rest/src/services/approval-service.ts` around lines 95 - 123,
`ApprovalService.approve` is marking proposals as EXECUTED even though
`broadcastProposal` only generates a mock hash and does not submit anything.
Update `approve()` to avoid returning execution success from the mock path, and
keep the proposal in a non-executed state until real broadcast logic exists;
also make sure `getProposal()` and `broadcastProposal()` remain consistent with
that interim behavior.
Comment on lines
+43
to
+79
| inviteMember(input: TeamInviteInput): TeamMember { | ||
| const email = input.email.trim().toLowerCase(); | ||
| if (!email) { | ||
| throw new Error('email is required'); | ||
| } | ||
|
|
||
| const duplicate = [...this.members.values()].find( | ||
| (member) => | ||
| member.organizationId === input.organizationId && member.email === email | ||
| ); | ||
| if (duplicate) { | ||
| throw new Error('Member already exists in organization'); | ||
| } | ||
|
|
||
| const member: TeamMember = { | ||
| id: `member_${Date.now()}_${Math.random().toString(36).slice(2, 8)}`, | ||
| organizationId: input.organizationId, | ||
| email, | ||
| role: input.role, | ||
| joinedAt: new Date().toISOString(), | ||
| }; | ||
|
|
||
| this.members.set(member.id, member); | ||
| this.appendActivity(input.organizationId, email, `Invited ${email} as ${input.role}`); | ||
| return member; | ||
| } | ||
|
|
||
| updateMemberRole(memberId: string, role: TeamRole): TeamMember { | ||
| const member = this.members.get(memberId); | ||
| if (!member) { | ||
| throw new Error('Member not found'); | ||
| } | ||
|
|
||
| member.role = role; | ||
| this.members.set(memberId, member); | ||
| this.appendActivity(member.organizationId, member.email, `Updated role to ${role}`); | ||
| return member; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Pass the real actor into activity logging.
inviteMember() and updateMemberRole() currently log the invited/updated member as actorEmail, so the workspace activity feed records the target as the person who performed the change. That breaks the audit trail this feature is meant to provide. Make these methods accept the authenticated actor and pass that through to appendActivity().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/rest/src/services/team-service.ts` around lines 43 - 79, The
activity logging in TeamService is using the target member’s email as the actor,
which makes the audit trail incorrect. Update inviteMember() and
updateMemberRole() to accept the authenticated actor identity as an additional
argument, then pass that actor through to appendActivity() instead of
input.email/member.email. Keep the existing member lookup and role update logic
intact, and use the same actor value wherever these methods record workspace
activity.
Comment on lines
+23
to
+25
| const threshold = Number(config.threshold); | ||
| if (!Number.isFinite(threshold)) { | ||
| throw new Error('threshold must be a numeric string'); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject blank thresholds before numeric coercion.
Line 23 currently accepts '' and ' ' as 0, so an invalid config can pass validation and evaluate against a zero threshold instead of throwing.
Proposed fix
async evaluate(config: PriceTriggerConfig): Promise<boolean> {
- const threshold = Number(config.threshold);
+ const normalizedThreshold = config.threshold.trim();
+ if (normalizedThreshold.length === 0) {
+ throw new Error('threshold must be a numeric string');
+ }
+ const threshold = Number(normalizedThreshold);
if (!Number.isFinite(threshold)) {
throw new Error('threshold must be a numeric string');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const threshold = Number(config.threshold); | |
| if (!Number.isFinite(threshold)) { | |
| throw new Error('threshold must be a numeric string'); | |
| const normalizedThreshold = config.threshold.trim(); | |
| if (normalizedThreshold.length === 0) { | |
| throw new Error('threshold must be a numeric string'); | |
| } | |
| const threshold = Number(normalizedThreshold); | |
| if (!Number.isFinite(threshold)) { | |
| throw new Error('threshold must be a numeric string'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/automation/src/triggers/price-trigger.ts` around lines 23 - 25,
The threshold validation in price-trigger’s numeric parsing currently lets blank
strings like '' and ' ' coerce to 0, so reject empty or whitespace-only values
before calling Number(config.threshold). Update the validation in the trigger’s
threshold handling to trim and check the raw config.threshold first, then only
coerce nonblank input and keep the existing finite-number guard. Make sure the
logic around the threshold setup in price-trigger.ts still throws for invalid
config and only accepts real numeric strings.
Comment on lines
+47
to
+63
| registerWallet(registration: SharedWalletRegistration): void { | ||
| const server = new Horizon.Server(this.horizonUrl); | ||
| const config: MultiSigConfig = { | ||
| ...registration.config, | ||
| networkPassphrase: | ||
| registration.config.networkPassphrase || this.defaultNetworkPassphrase, | ||
| }; | ||
|
|
||
| const signerWeights = new Map( | ||
| config.signers.map((signer) => [signer.publicKey, signer.weight]) | ||
| ); | ||
|
|
||
| this.wallets.set(registration.walletAddress, { | ||
| wallet: new MultiSigWallet(server, config), | ||
| organizationId: registration.organizationId, | ||
| signerWeights, | ||
| }); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject duplicate wallet registration instead of overwriting state.
Map#set keyed only by walletAddress replaces the existing MultiSigWallet, dropping pending proposals/signatures and silently remapping the wallet to another organization. Guard duplicates or use an explicit update path.
Proposed guard
registerWallet(registration: SharedWalletRegistration): void {
+ const existing = this.wallets.get(registration.walletAddress);
+ if (existing) {
+ throw new Error(`Shared wallet ${registration.walletAddress} is already registered`);
+ }
+
const server = new Horizon.Server(this.horizonUrl);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| registerWallet(registration: SharedWalletRegistration): void { | |
| const server = new Horizon.Server(this.horizonUrl); | |
| const config: MultiSigConfig = { | |
| ...registration.config, | |
| networkPassphrase: | |
| registration.config.networkPassphrase || this.defaultNetworkPassphrase, | |
| }; | |
| const signerWeights = new Map( | |
| config.signers.map((signer) => [signer.publicKey, signer.weight]) | |
| ); | |
| this.wallets.set(registration.walletAddress, { | |
| wallet: new MultiSigWallet(server, config), | |
| organizationId: registration.organizationId, | |
| signerWeights, | |
| }); | |
| registerWallet(registration: SharedWalletRegistration): void { | |
| const existing = this.wallets.get(registration.walletAddress); | |
| if (existing) { | |
| throw new Error(`Shared wallet ${registration.walletAddress} is already registered`); | |
| } | |
| const server = new Horizon.Server(this.horizonUrl); | |
| const config: MultiSigConfig = { | |
| ...registration.config, | |
| networkPassphrase: | |
| registration.config.networkPassphrase || this.defaultNetworkPassphrase, | |
| }; | |
| const signerWeights = new Map( | |
| config.signers.map((signer) => [signer.publicKey, signer.weight]) | |
| ); | |
| this.wallets.set(registration.walletAddress, { | |
| wallet: new MultiSigWallet(server, config), | |
| organizationId: registration.organizationId, | |
| signerWeights, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/wallet/src/shared-wallet.service.ts` around lines 47 - 63,
Reject duplicate wallet registrations in registerWallet rather than letting
this.wallets.set overwrite an existing entry keyed by walletAddress. Add a guard
before creating the new MultiSigWallet so repeated registrations for the same
walletAddress are ignored or fail explicitly, and keep the current
organizationId, signerWeights, and wallet state intact. If updates are intended,
route them through a separate explicit update path instead of reusing
registerWallet.
Comment on lines
+98
to
+138
| body.innerHTML = this.members | ||
| .map( | ||
| (member) => ` | ||
| <tr data-member-id="${member.id}"> | ||
| <td>${member.email}</td> | ||
| <td> | ||
| <select class="tm-role-select" data-member-id="${member.id}" aria-label="Role for ${member.email}"> | ||
| <option value="admin" ${member.role === 'admin' ? 'selected' : ''}>Admin</option> | ||
| <option value="member" ${member.role === 'member' ? 'selected' : ''}>Member</option> | ||
| <option value="viewer" ${member.role === 'viewer' ? 'selected' : ''}>Viewer</option> | ||
| </select> | ||
| </td> | ||
| <td>${new Date(member.joinedAt).toLocaleString()}</td> | ||
| <td><button class="tm-save-role" data-member-id="${member.id}">Save Role</button></td> | ||
| </tr> | ||
| ` | ||
| ) | ||
| .join(''); | ||
|
|
||
| body.querySelectorAll('.tm-save-role').forEach((button) => { | ||
| button.addEventListener('click', (event) => { | ||
| const target = event.currentTarget as HTMLButtonElement; | ||
| void this.handleRoleUpdate(target.dataset.memberId!); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| private async renderActivity(): Promise<void> { | ||
| const list = document.getElementById('tm-activity-list'); | ||
| if (!list) return; | ||
|
|
||
| const activity = await this.client.listActivity(this.organizationId); | ||
| list.innerHTML = | ||
| activity.length === 0 | ||
| ? '<li>No activity yet.</li>' | ||
| : activity | ||
| .map( | ||
| (entry) => | ||
| `<li><strong>${entry.actorEmail}</strong> — ${entry.action} <em>(${new Date(entry.createdAt).toLocaleString()})</em></li>` | ||
| ) | ||
| .join(''); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Don't render API data through innerHTML here.
The backend accepts any string for email, and TeamService stores that value in both member.email and the activity text. Interpolating member.email, entry.actorEmail, and entry.action into HTML makes this a stored XSS path. Build these rows with DOM APIs / textContent (or sanitize before insertion).
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 129-137: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: list.innerHTML =
activity.length === 0
? '
: activity
.map(
(entry) =>
<li><strong>${entry.actorEmail}</strong> — ${entry.action} <em>(${new Date(entry.createdAt).toLocaleString()})</em></li>)
.join('')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(dom-content-modification)
[warning] 129-137: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: list.innerHTML =
activity.length === 0
? '
: activity
.map(
(entry) =>
<li><strong>${entry.actorEmail}</strong> — ${entry.action} <em>(${new Date(entry.createdAt).toLocaleString()})</em></li>)
.join('')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(unsafe-html-content-assignment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/frontend/src/panels/team-management.ts` around lines 98 - 138, The
team management rendering in renderMembers and renderActivity is injecting
API-provided strings through innerHTML, which creates a stored XSS path via
member.email, entry.actorEmail, and entry.action. Rework these DOM updates to
build the table rows and activity items with DOM APIs and textContent (or
sanitize the values before insertion), and keep the existing
renderMembers/renderActivity flow and element selectors intact.
Contributor
Author
|
Linking all implementation issues for tracking:
|
Contributor
|
@josephchimebuka fix the ci |
…workflows-301-315-316-317
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
2 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
This PR implements four Phase 3/4 roadmap features for enterprise and automation workflows:
PriceTrigger): Addspackages/core/automation/src/triggers/price-trigger.tswithOracleAggregatorintegration so automation rules can fire when asset-pair prices cross configurable above/below thresholds.TeamManagementPanelwith member listing, invitation flow, RBAC role dropdowns, and workspace activity logs. Wired into the playground sidebar and backed by new/api/v1/teamsroutes.SharedWalletServicemapping multi-sig vaults to organization workspaces withproposeTx/signTx, signature aggregation until threshold is met, and unit tests.POST /api/v1/approvals/proposeandPOST /api/v1/approvals/approvewith validation, invalid-signature rejection, and broadcast only when the multi-sig threshold is satisfied.Changes by area
packages/core/automation/src/triggers/price-trigger.tspackages/core/wallet/src/shared-wallet.service.tspackages/api/rest/src/routes/approvals/,packages/api/rest/src/routes/teams/, servicespackages/frontend/src/panels/team-management.ts,team-management.client.tsTest plan
SharedWalletService— proposes templates, aggregates signatures, rejects invalid signaturesPriceTrigger— evaluates above/below thresholds via mockedOracleAggregatorTeamManagementPanel— renders members/activity, submits invite via team client/api/v1/approvals/proposethen/api/v1/approvals/approvewith valid Stellar signaturesLinked issues
Closes #301
Closes #315
Closes #316
Closes #317
Relates to #237
Relates to #268
Relates to #283
Summary by CodeRabbit
New Features
Bug Fixes