-
Notifications
You must be signed in to change notification settings - Fork 137
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 helpers to create CAP23 operations. #368
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.
Again, I'm not a JS expert by any means, but everything LGTM 👌
* | ||
*/ | ||
export function claimClaimableBalance(opts) { | ||
if (typeof opts.balanceId !== 'string' || opts.balanceId.length < 2) { |
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.
Wait, why length < 2?
Also, the docstring above says opts.claimableBalanceId
which doesn't match opts.balanceId
here.
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.
good catch! Actually we don't need the length check, we can delegate that to the XDR library.
CHANGELOG.md
Outdated
|
||
```js | ||
const op = Operation.createClaimableBalance({ | ||
balanceId: '0da0d57da7d4850e7fc10d2a9d0ebc731f7afb40574c03395b17d49149b91f5be', |
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 we decided not to compress the XDR, this should be updated with the leading 0s!
throw new TypeError(this.constructAmountRequirementsError('amount')); | ||
} | ||
|
||
if (!opts.claimants || (opts.claimants && opts.claimants.length === 0)) { |
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.
Doesn't the latter condition fulfill the former condition?
That is, if opts.claimants = []
, then I think !opts.claimants
will be true.
Maybe a type check for Array
makes sense here, instead?
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.
good point, updated to. (!Array.isArray(opts.claimants) || opts.claimants.length === 0)
Extend the
Operation
class with two new helpers which allow the creation of CAP23 related operations:Operation.createClaimableBalance
.Extend the operation class with a new helper to create claimable balance operations.
Operation.claimClaimableBalance
.Extend the operation class with a new helper to create claim claimable balance operations. It receives the
balanceId
as exposed by Horizon in the/claimable_balances
end-point.