-
Notifications
You must be signed in to change notification settings - Fork 299
feat(sdk-coin-hbar): add token enablement transaction verification #7003
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
base: master
Are you sure you want to change the base?
Conversation
}); | ||
}); | ||
|
||
describe('Verify Token Enablement Transaction:', () => { |
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.
let's add a test that uses a valid transaction response from wallet platform with a spoofed txHex,
the test should have the structure of
await assert.rejects(
wallet.sendTokenEnablements(...)
)...
so we know a spoofed tx hex will throw an error on this flow.
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.
also another test verifying a valid token enablement tx won't cause an error.
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.
Tests on 685-725 and 726-774 are the two tests that address this.
8b832e3
to
e3e03ab
Compare
564c306
to
a90a4e7
Compare
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.
requesting a review from the other coin owner.
9eddfef
to
acd50cf
Compare
}); | ||
}); | ||
|
||
describe('Verify Token Enablement Transaction:', () => { |
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.
also another test verifying a valid token enablement tx won't cause an error.
acd50cf
to
43e27d0
Compare
b638a30
to
c7481dd
Compare
8ef38d8
to
6eb45c0
Compare
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.
generally LGTM, some comments to follow up on.
modules/sdk-coin-hbar/src/hbar.ts
Outdated
private validateNoTransfers(raw: HederaRawTransactionData): void { | ||
if (raw.instructionsData?.params?.recipients?.length && raw.instructionsData.params.recipients.length > 0) { | ||
const hasNonZeroTransfers = raw.instructionsData.params.recipients.some( | ||
(recipient: any) => recipient.amount && recipient.amount !== '0' |
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.
please address the typing in a follow up.
modules/sdk-coin-hbar/src/hbar.ts
Outdated
const t = String(raw.instructionsData?.type || '').toLowerCase(); | ||
|
||
if (t === 'contractexecute' || t === 'contractcall' || t === 'precompile') { | ||
throw new Error(`Contract-based token association not allowed for blind enablement; got ${t}`); | ||
} | ||
|
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.
please update the variable naming here.
6377c0b
to
f65d214
Compare
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.
Please squash all the commits into a single commit.
- Add validation functions for transaction structure, amount, account ID, token name, and transaction type - Integrate token enablement validation into verifyTransaction method - Add test coverage for all validation scenarios TICKET: WP-5746
f65d214
to
2136fff
Compare
Add verifyTokenEnablementTransaction method to validate token associate transactions with comprehensive checks for amount, account ID, token name, and transaction type. Integrates with existing verifyTransaction flow for enhanced token enablement validation.
TICKET: WP-5746