Conversation
gaurangtorvekar
requested review from
HGraphPunks and
kantorcodes
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
| /** Schedule delete of a Flora account (AccountDeleteTransaction). */ | ||
| export function buildHcs16ScheduleAccountDeleteTx(params: { | ||
| floraAccountId: string; | ||
| transferAccountId: string; | ||
| memo?: string; | ||
| }): ScheduleCreateTransaction { | ||
| const inner = new AccountUpdateTransaction() as any; // AccountDeleteTransaction not imported in current SDK typings set | ||
| // Fallback: construct via new proto if needed in future; here we use typed import when available | ||
| // Replace when AccountDeleteTransaction is exported in the used SDK version | ||
| (inner as any).setAccountId?.(AccountId.fromString(params.floraAccountId)); | ||
| if (params.memo) { | ||
| (inner as any).setTransactionMemo?.(params.memo); | ||
| } | ||
| (inner as any).setTransferAccountId?.( | ||
| AccountId.fromString(params.transferAccountId), | ||
| ); | ||
| return new ScheduleCreateTransaction().setScheduledTransaction(inner); |
There was a problem hiding this comment.
The new buildHcs16ScheduleAccountDeleteTx builds the scheduled transaction from an AccountUpdateTransaction and conditionally calls setTransferAccountId and other delete-only setters via optional chaining. Because AccountUpdateTransaction does not implement these methods, none of them execute and the scheduled transaction ends up as an empty account update rather than an AccountDeleteTransaction. Any call to scheduleFloraDeletion will therefore submit a schedule that never deletes the Flora account and omits the mandatory transfer account, causing execution to fail. Use a proper AccountDeleteTransaction (or construct the delete proto directly) so the schedule actually performs an account deletion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed this
|
@gaurangtorvekar it looks like the DCO check is still failing. Either the GPG key is not working or -s was not used with the commits. You might have to do a squash / rebase |
src/hcs-16/base-client.ts
Outdated
There was a problem hiding this comment.
We have a key-detector class here that might be helpful https://github.com/hashgraph-online/standards-sdk/blob/main/src/utils/key-type-detector.ts
src/hcs-16/sdk.ts
Outdated
There was a problem hiding this comment.
We should have this transaction generated in tx.ts - the main reason we started doing this recently is so standards-agent-kit can reuse a transaction assembled in the right format without executing it immediately for use of the builder pattern.
src/hcs-16/sdk.ts
Outdated
There was a problem hiding this comment.
Do we need type any here, should we be enforcing the profile shape with zod?
src/hcs-16/tx.ts
Outdated
There was a problem hiding this comment.
Zod could be good for these type checks too potentially
Signed-off-by: Gaurang Torvekar <2285764+gaurangtorvekar@users.noreply.github.com>
gaurangtorvekar
force-pushed
the
feat/hcs-16-full-implementation
branch
from
1c5871d to
d0e12cc
Compare
|
@kantorcodes Made changes as per your comments, and now the DCO check is also passing |
2 participants
No description provided.