Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Branded token and Gateway composer interact with unit tests. #99
Branded token and Gateway composer interact with unit tests. #99
Changes from 8 commits
df51164
4dc3bd8
ddffa37
1c00e25
cdac015
756d3a1
1b44297
54cc372
56142bd
54411c9
621c488
56ffde7
2114826
5e62ba5
8d44367
630df14
053d514
516acd7
0e26e87
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should we call web3 => "originWeb3" as BT is deployed on origin chain?
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.
In my opinion, the
BrandedToken
class is not concerned with what's origin or auxiliary. It only cares that it is deployed on a chain that it can access with a web3 object.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.
no, rather moving it to
web3
from where it is specified (and ambivalent)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.
unit test for deploy and deployRawTx method is missing 👁
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.
Since the validations for typechecks are repetitive, I think we should create Validation library/util where we can refactor the code. Validation library/util can also be used in mosaic.js and openst.js also. @schemar What do you think?
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.
That could help 👍
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 value to which ... is set" is not a very valuable documentation. Instead, it should explain what the parameters are for/explain them.
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 can add a
@dev
note in the document explaining conversion rate and decimal.Does this work for you?
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 don't find
@dev
as a valid JSDoc annotation. It would also not be correct as this info is not intended for developers, but for consumers of the function and should therefore not be hidden behind@dev
.Why don't you explain the parameters where they're supposed to be explained?
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.
The check should be "<=" because ConversionRateDecimals can be less/equal to 5.
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 this be injected instead of hard coded? Probably in the constructor?
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 a static method, so we can't pass via constructor.
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.
Then pass it as an argument to the static method. Alternatively pass the
bin
.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 believe requestStake interface should be aligned with BrandedToken.requestStake.
So mintAmount should not be calculated in this function. It causes an extra geth call and updates the interfaces. Staker can call convertToBrandedTokens to know the mint amount.
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.
Why
Promise.resolve
and not simply: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.
Again why
Promise.resolve
?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.
Why Promise?
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 to make it consistent with
mosaic.js
.In
mosaic.js
all RawTx methods returns a promise, all repos should be same I think 🤔Refer this
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.
Why do we invent Promises when there are none? That just makes usability harder for the consumer. It seems we still have quite some mess in mosaic.js as well, where we did not review properly I assume. Same for AsyncAssert and Spy ...
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.
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.
Again why
Promise
?