-
Notifications
You must be signed in to change notification settings - Fork 300
Wp 5782/fix near token enablement validation #6958
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
3371efc
to
b81a0d0
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.
A few changes requested but you got a general first idea, good job! let me know if you need further clarification on any of the mentioned points @SimonVutovB
Thanks!
8d0fb13
to
fa835c7
Compare
@SimonVutovB for this issue that you're getting on the CI/CD steps, just rebase master since your dep versions are out of date: ![]() |
fa835c7
to
cf059f2
Compare
cf059f2
to
6141074
Compare
e9cbf28
to
10c27f8
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.
add a unit test to show sendTokenEnablements
throws an error when you mock a response with a spoofed TxHex. Otherwise lgtm.
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 add a test case showcasing sendAccountConsolidations
fails when a spoofed txHex is returned in the response.
f4a41a4
to
3be12f4
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.
Can we add a test that shows a spoofed tx from platform will be rejected, similar to HBAR's?
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.
i'll also request a review from the coin owners.
modules/sdk-coin-near/src/near.ts
Outdated
const explainedTx = transaction.explainTransaction(); | ||
|
||
// users do not input recipients for consolidation requests as they are generated by the server | ||
if (txParams.type === 'enabletoken') { |
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.
if (txParams.type === 'enabletoken') { | |
if (txParams.type === 'enabletoken' && verification.verifyTokenEnablement) { |
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.
LGTM, missing one last change.
modules/sdk-coin-near/src/near.ts
Outdated
|
||
if (!_.isEqual(filteredOutputs, filteredRecipients)) { | ||
// For enabletoken, provide more specific error messages for address mismatches | ||
if (txParams.type === 'enabletoken') { |
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.
if (txParams.type === 'enabletoken') { | |
if (txParams.type === 'enabletoken' && params.verification?.verifyTokenEnablement) { |
Not sure if we always want to throw this new error or only if the param is passed?
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.
Yeah you're right, good catch. Just pushed change to add the check for the verifyTokenEnablement flag.
8f353e7
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.
NEAR token enablement blind signing validation TICKET: WP-5782
8f353e7
to
d84bba2
Compare
PR has merge conflicts |
307041c
57d600d
to
3e49289
Compare
lerna.json
Outdated
{ | ||
"version": "independent", | ||
"npmClient": "yarn", | ||
"packages": ["modules/*"], |
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.
is this needed?
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.
I deleted it and all checks pass, so I guess it was not needed.
// The result should contain failures due to the spoofed transaction hex | ||
result.success.should.have.length(0); | ||
result.failure.should.have.length(1); | ||
result.failure[0].message.should.containEql('unable to build transaction from raw'); |
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.
I'm not sure how this is testing that the spoofed transaction does not match the intended 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.
Yeah I agree with @mohammadalfaiyazbitgo , I don't remember how the internal flow goes exactly but if you mock the build results then you're basically shutting down the internal verifies somewhere.
@SimonVutovB mind trying an approach similar to what I did in this other tests? It's also a little shorter and you may throw an error msg that should be the same as one of your token enablement error msg's on the current PR:
For ref, this is on modules/sdk-coin-xlm/test/unit/xlm.ts , look for the test name on that file please.
const output = explainedTx.outputs[0]; | ||
const recipient = txParams.recipients?.[0]; | ||
|
||
if (!recipient?.address) { |
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.
Could you confirm if near token enablements supports a single recipient? because if that's the case then you may need to check for outputs.length === 1 and if it isn't then you may need to iterate over the outputs or recipients and match one with the other (also you can preliminary compare outputs.length with recipients.length and they should match).
// The result should contain failures due to the spoofed transaction hex | ||
result.success.should.have.length(0); | ||
result.failure.should.have.length(1); | ||
result.failure[0].message.should.containEql('unable to build transaction from raw'); |
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.
Yeah I agree with @mohammadalfaiyazbitgo , I don't remember how the internal flow goes exactly but if you mock the build results then you're basically shutting down the internal verifies somewhere.
@SimonVutovB mind trying an approach similar to what I did in this other tests? It's also a little shorter and you may throw an error msg that should be the same as one of your token enablement error msg's on the current PR:
For ref, this is on modules/sdk-coin-xlm/test/unit/xlm.ts , look for the test name on that file please.
Unit tests for tokenEnablementValidation changes and Making sure there is 1 user in the txParams.recipients TICKET: WP-5782
Summary
Added token enablement validation and tests
Changes
verifyTransaction
in near.ts of sdk-coin-near, validate the txHex is a valid token enablement transaction for the specified token and does not have additional transactions embedded.TICKET: WP-5782