-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
Add BigNumber support to expectEvent/inLogs (#1026) #1338
Add BigNumber support to expectEvent/inLogs (#1026) #1338
Conversation
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.
Thanks @jbogacz, this is great!
I have a couple questions:
-
is there a way to make this work without having to explicitly pass a
BigNumber
? e.g.{ id: 1 }
instead of{ id: new BigNumber(1) }
-
Can this be done without using
lodash
? We're in the process of removing it from some test files (Remove lodash from tests #1323), though it may be fine to leave it here.
* switched direct logs array check to expectEvent method in AllowanceCrowdsale.test.js
48fae63
to
aff6533
Compare
Hey, I've pushed my recent changes as follow:
I will take care #1232 nextly. |
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.
Looking great @jbogacz! This will be extremely helpful, thanks!
I left a minor comment we should address before merging.
test/helpers/expectEvent.js
Outdated
@@ -27,6 +27,16 @@ async function inTransaction (tx, eventName, eventArgs = {}) { | |||
return inLogs(logs, eventName, eventArgs); | |||
} | |||
|
|||
function toSimpleValue (value) { | |||
return isBigNumber(value) | |||
? value.toNumber() |
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.
This may cause issues for large numbers, which JavaScript won't be able to properly represent. What if instead of converting to JS number types, we instead used should.equal
and should.be.bignumber.equal
respectively?
Hey @nventuro , please take a look my recent update. To be honest I'm not so experienced with JS and more advanced import technics are tricky for me. Please take a look into chai-bignumber.js library, there is already |
2d79786
to
9058931
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.
This is great @jbogacz, thanks a lot! I think copying chai-bignumber
's code is fine for now, since I don't see a way to call it from the outside, and we can always improve the helper later if needed :)
Cool! Thanks! |
* Add BigNumber support to expectEvent/inLogs (#1026) * switched direct logs array check to expectEvent method in AllowanceCrowdsale.test.js * Refactor expectEvent.inLogs function to use simple value number check * Introduced should.be.bignumber method to compare BigNumber values * Destructure transaction object to extract logs field (cherry picked from commit 947de54)
🚀 Description
Condition for equality check in expectEvent.inLogs() method was changed to verify equality even compared objects are structs. It fixes issue where BigNumber emitted to event was not possible to validate as follow:
const event = expectEvent.inLogs(currentSnapshotId.logs, 'Snapshot', { id: new BigNumber(1) });
Now it works fine.
npm run lint:fix
).