-
Notifications
You must be signed in to change notification settings - Fork 6
Fix sdk no broadcaster, wallet parts, claim #614
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
Conversation
📝 WalkthroughWalkthroughThis PR refactors authentication prioritization logic for Hive Engine reward claims, giving HiveAuth precedence when applicable, restructures SDK auth broadcasting to check HiveAuth first, adds support for Claim operations in wallet mutations, extends asset info with component breakdown, and bumps the wallets package version. Changes
Sequence DiagramsequenceDiagram
actor User
participant Button as Claim Rewards Button
participant Auth as Auth Context (SDK)
participant HiveAuth as HiveAuth
participant Keychain as Keychain
participant Engine as Hive Engine
User->>Button: Click Claim Rewards
Button->>Button: Evaluate signType priority
alt shouldUseHiveAuth() == true
Button->>Auth: Use HiveAuth
Auth->>HiveAuth: Configure broadcast for active/posting
HiveAuth->>Engine: Sign & execute claim
else loginType == "keychain"
Button->>Auth: Use Keychain
Auth->>Keychain: Configure broadcast with authority mapping
Keychain->>Engine: Sign & execute claim
else
Button->>Auth: Use HiveSigner (default)
Auth->>Engine: Execute claim
end
Engine-->>User: Return claim result
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/wallets/src/modules/wallets/queries/get-account-wallet-asset-info-query-options.ts (1)
80-93: Inconsistent handling of zero balances between liquid and staked/savings.The
liquidfield is included inpartseven when its value is0(only checks forundefined/null), whilestakedandsavingsrequire> 0to be included. This asymmetry may be intentional (liquid balance of 0 is meaningful context), but verify this is the desired behavior.If consistency is preferred:
Option A: Include liquid only when > 0 (like staked/savings)
- if (assetInfo.liquid !== undefined && assetInfo.liquid !== null) { + if (assetInfo.liquid !== undefined && assetInfo.liquid !== null && assetInfo.liquid > 0) { parts.push({ name: "liquid", balance: assetInfo.liquid }); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
packages/wallets/dist/browser/index.jsis excluded by!**/dist/**packages/wallets/dist/browser/index.js.mapis excluded by!**/dist/**,!**/*.mappackages/wallets/dist/node/index.cjsis excluded by!**/dist/**packages/wallets/dist/node/index.cjs.mapis excluded by!**/dist/**,!**/*.mappackages/wallets/dist/node/index.mjsis excluded by!**/dist/**packages/wallets/dist/node/index.mjs.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (5)
apps/web/src/app/(dynamicPages)/profile/[username]/wallet/(token)/[token]/_components/hive-engine-claim-rewards-button.tsxapps/web/src/utils/sdk-auth.tspackages/wallets/package.jsonpackages/wallets/src/modules/wallets/mutations/wallet-operation.tspackages/wallets/src/modules/wallets/queries/get-account-wallet-asset-info-query-options.ts
🧰 Additional context used
🧬 Code graph analysis (4)
packages/wallets/src/modules/wallets/mutations/wallet-operation.ts (4)
packages/wallets/dist/browser/index.d.ts (2)
AssetOperation(2031-2031)claimHiveEngineRewards(2031-2031)packages/wallets/dist/node/index.mjs (11)
payload(1320-1320)payload(1373-1373)payload(1398-1398)payload(1538-1538)payload(1625-1625)payload(1672-1672)payload(1715-1715)payload(1762-1762)payload(2801-2801)payload(3045-3045)auth(373-373)packages/wallets/dist/node/index.cjs (7)
payload(1347-1347)payload(1400-1400)payload(1425-1425)payload(1565-1565)payload(1652-1652)payload(1699-1699)auth(400-400)packages/wallets/dist/browser/index.js (1)
auth(375-375)
apps/web/src/app/(dynamicPages)/profile/[username]/wallet/(token)/[token]/_components/hive-engine-claim-rewards-button.tsx (1)
apps/web/src/utils/hive-auth.ts (1)
shouldUseHiveAuth(1351-1363)
packages/wallets/src/modules/wallets/queries/get-account-wallet-asset-info-query-options.ts (2)
packages/wallets/dist/node/index.mjs (4)
parts(2201-2204)parts(3596-3596)assetInfo(3592-3594)assetInfo(3627-3627)packages/wallets/dist/node/index.cjs (4)
parts(2228-2231)parts(3623-3623)assetInfo(3619-3621)assetInfo(3654-3654)
apps/web/src/utils/sdk-auth.ts (1)
apps/web/src/utils/hive-auth.ts (2)
shouldUseHiveAuth(1351-1363)broadcastWithHiveAuth(1374-1381)
🔇 Additional comments (7)
packages/wallets/package.json (1)
4-4: LGTM!The patch version bump is appropriate for the bug fixes and new Claim operation support added in this PR.
packages/wallets/src/modules/wallets/queries/get-account-wallet-asset-info-query-options.ts (1)
95-104: LGTM!The
GeneralAssetInfoobject is well-constructed with the newpartsfield providing useful balance breakdown information for UI components.packages/wallets/src/modules/wallets/mutations/wallet-operation.ts (1)
73-84: LGTM!The
Claimoperation handler correctly transforms the generic wallet operation payload into the format expected byclaimHiveEngineRewards:
- Maps
payload.from→account- Wraps
payload.assetin an array fortokens- Conditionally includes the
keyonly whentypeis"key"This aligns well with the claim flow in
hive-engine-claim-rewards-button.tsx.apps/web/src/utils/sdk-auth.ts (2)
26-41: LGTM!The keychain broadcast setup is now properly guarded with an explicit
else ifcheck, ensuring it only applies when the user explicitly logged in with keychain. The authority mapping logic is unchanged and correct.
17-25: No action needed—shouldUseHiveAuthproperly respects explicit keychain login.The function checks the stored
loginTypefirst viagetLoginType(username)and returnsfalseifloginType === "keychain"(since it only returnstruewhenloginType === "hiveauth"). Browser detection only applies as a fallback when no stored login type exists. The edge case is safely handled—explicit keychain login will not be overridden by HiveAuth.apps/web/src/app/(dynamicPages)/profile/[username]/wallet/(token)/[token]/_components/hive-engine-claim-rewards-button.tsx (2)
161-168: LGTM!The
signTypeselection logic now correctly aligns with the prioritization insdk-auth.ts:
- HiveAuth takes precedence via
shouldUseHiveAuth()- Keychain only when explicitly
loginType === "keychain"- Defaults to hivesigner otherwise
This ensures consistent authentication behavior across the claim workflow.
170-182: LGTM!The claim mutation correctly handles both authentication paths:
- Direct key signing for
privateKeylogin type- Delegated signing via
signTypeandauthcontext for other login typesThe flow properly leverages the
claimHiveEngineRewardsfunction from the wallets package.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.