Skip to content
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

Use BN for GenericScheme value #550

Merged
merged 12 commits into from
Oct 27, 2020
Merged

Use BN for GenericScheme value #550

merged 12 commits into from
Oct 27, 2020

Conversation

dkent600
Copy link
Contributor

@dkent600 dkent600 commented Oct 20, 2020

Resolves: #549

@dkent600 dkent600 changed the title BNForGenericSchemeValue Use BN for GenericScheme value Oct 20, 2020
@@ -18,7 +19,7 @@ export interface IUGenericScheme {

export interface IProposalCreateOptionsGS {
callData?: string
value?: number
value?: BN | number | string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this cannot be number

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because value can be bigger than number

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...in which case the calling application would already have crashed before getting into this code. This code can handle any number that an app is able to send to it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please add a test which send Number.MAX_SAFE_INTEGER +1 as the value ?
MAX_SAFE_INTEGER =. 2^53-1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can tell you what will happen in your proposed case with the code as I've written it. The result will be that unexpected incorrect numbers will be sent to the contract (see the screenshot below).

There is nothing we can do to prevent the user from submitting numbers like this. We also can't prevent them from submitting totally invalid string representations. We could validate the values before submitting them to the contract, but note that we don't do that anywhere else where quantities are accepted, so why start here? We leave it up to the application to validate user input.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please submit your test. we should be able to support these kind of big values .
e.g sending 1 ether - 1000000000000000000 .

Copy link
Contributor Author

@dkent600 dkent600 Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the existing test to use new BN('1000000000000000000')

@orenyodfat orenyodfat closed this Oct 27, 2020
@orenyodfat orenyodfat reopened this Oct 27, 2020
@orenyodfat orenyodfat closed this Oct 27, 2020
@orenyodfat orenyodfat reopened this Oct 27, 2020
@orenyodfat orenyodfat closed this Oct 27, 2020
@orenyodfat orenyodfat reopened this Oct 27, 2020
.travis.yml Outdated
sudo: required
sudo: required
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this ?

orenyodfat
orenyodfat previously approved these changes Oct 27, 2020
@orenyodfat orenyodfat merged commit e476564 into master Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

genericscheme value should be BN (not number)
2 participants