Skip to content

feat: implement wallet_getSubAccounts rpc #1676

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jul 8, 2025
Merged

feat: implement wallet_getSubAccounts rpc #1676

merged 5 commits into from
Jul 8, 2025

Conversation

@montycheese
Copy link
Contributor

@montycheese montycheese commented Jul 3, 2025 •
edited
Loading

Summary

Integrate with wallet_getSubAccounts RPC

How did you test your changes?

Screenshot 2025-07-03 at 3 07 37 PM

Created section on playground to test RPC against dev endpoint implementation

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Jul 3, 2025 •
edited
Loading

✅ Heimdall Review Status

Requirement Status More Info
Reviews ✅ 2/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@montycheese montycheese force-pushed the montanawong/ba-2308-sdk-implement-wallet_getsubaccounts-rpc branch from b73f805 to f223d00 Compare July 3, 2025 19:36
@montycheese montycheese marked this pull request as ready for review July 3, 2025 19:56
stephancill
stephancill reviewed Jul 3, 2025
View reviewed changes
Copy link
Contributor

@stephancill stephancill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the response not be just SubAccount[] vs {subAccounts: SubAccount[]}?

stephancill
stephancill reviewed Jul 3, 2025
View reviewed changes
Newportgogogadit
Newportgogogadit reviewed Jul 5, 2025
View reviewed changes
Copy link

@Newportgogogadit Newportgogogadit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙃

@montycheese montycheese force-pushed the montanawong/ba-2308-sdk-implement-wallet_getsubaccounts-rpc branch from d9efdda to 4c662b7 Compare July 7, 2025 22:44
@montycheese montycheese force-pushed the montanawong/ba-2308-sdk-implement-wallet_getsubaccounts-rpc branch from 4c662b7 to 4709c7d Compare July 7, 2025 23:23
@montycheese montycheese requested a review from stephancill July 7, 2025 23:28
fan-zhang-sv
fan-zhang-sv reviewed Jul 7, 2025
View reviewed changes
@montycheese montycheese requested a review from fan-zhang-sv July 7, 2025 23:41
stephancill
stephancill previously approved these changes Jul 8, 2025
View reviewed changes
@montycheese montycheese requested a review from cb-jake July 8, 2025 14:43
cb-jake
cb-jake reviewed Jul 8, 2025
View reviewed changes
cb-jake
cb-jake reviewed Jul 8, 2025
View reviewed changes
packages/wallet-sdk/src/sign/scw/SCWSigner.ts
Comment on lines +292 to +294
if (!this.chain.rpcUrl) {
throw standardErrors.rpc.internal('No RPC URL set for chain');
}
Copy link
Contributor

@cb-jake cb-jake Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably missed the design reviews for getSubAccounts but should the RPC take a chainID?

Copy link
Contributor Author

@montycheese montycheese Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

current BE implementation dedupes them on network so only one sub account is returned per address so chain ID is not required. The current approach is that sub accounts will feel cross-chain as the SDK/keys will perform JIT sub account deployments for crosschain transactions, and also perform JIT signer updates if the local signer is not on a given chain. We can revisit if we need to provide explicit chain id querying here in the future as well

cb-jake
cb-jake previously approved these changes Jul 8, 2025
View reviewed changes
Copy link
Contributor

@cb-jake cb-jake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, curious about chain id being a param

@montycheese montycheese dismissed stale reviews from cb-jake and stephancill via b8c2a60 July 8, 2025 16:02
@montycheese montycheese merged commit 811c879 into master Jul 8, 2025
8 checks passed
@montycheese montycheese deleted the montanawong/ba-2308-sdk-implement-wallet_getsubaccounts-rpc branch July 8, 2025 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stephancill stephancill stephancill approved these changes

@cb-jake cb-jake cb-jake approved these changes

@fan-zhang-sv fan-zhang-sv Awaiting requested review from fan-zhang-sv

+1 more reviewer

@Newportgogogadit Newportgogogadit Newportgogogadit left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@montycheese @cb-heimdall @stephancill @fan-zhang-sv @cb-jake @Newportgogogadit