-
Notifications
You must be signed in to change notification settings - Fork 70
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
Set maximum WG budget increase amount (#2327) #3920
base: dev
Are you sure you want to change the base?
Set maximum WG budget increase amount (#2327) #3920
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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
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.
@traumschule here:
You identified a bug where a javascript number was passed to asUpdateWorkingGroupBudget
instead of a BN
, I'm not sure why the type system doesn't detect this, but it's probably because somehow the fragment.amount
is typed any
.
Here's an example of BN
with values > Number.MAX_SAFE_INTEGER
): https://codepen.io/thesan/pen/poKmpzb?editors=0012
That said on chain most amount are encoded as u64
and currently the default maxAllowedValue
of <TokenInput />
is powerOf2(128)
so it should be changed to powerOf2(64)
budgetUpdate: BNSchema.test(moreThanMixed(0, 'Amount must be greater than zero')).required('Field is required'), | ||
budgetUpdate: BNSchema.test(moreThanMixed(0, 'Amount must be greater than zero.')) | ||
.test(maxMixed(new BN(999999999999999), 'Amount must be less than 99999.', true)) | ||
.required('Field is required'), |
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.
@traumschule please remove this change. The budget can be over this amount and it won't cause any issue since: #4018.
FYI Zeeshan recently solved the underlying cause in Hydra here: Joystream/hydra#515. I'm not sure if this is deployed yet but either way everything will be fine as long as the value is less than 2⁶⁴
#2327
BN seems to have a limit of 10000000000000000.