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

Add helpers to create CAP23 operations. #368

Merged
merged 8 commits into from
Sep 24, 2020
Merged

Add helpers to create CAP23 operations. #368

merged 8 commits into from
Sep 24, 2020

Conversation

abuiles
Copy link
Contributor

@abuiles abuiles commented Sep 23, 2020

Extend the Operation class with two new helpers which allow the creation of CAP23 related operations:

  • Add Operation.createClaimableBalance.
    Extend the operation class with a new helper to create claimable balance operations.
const asset = new Asset(
  'USD',
  'GDGU5OAPHNPU5UCLE5RDJHG7PXZFQYWKCFOEXSXNMR6KRQRI5T6XXCD7'
);
const amount = '100.0000000';
const claimants = [
  new Claimant(
    'GCEZWKCA5VLDNRLN3RPRJMRZOX3Z6G5CHCGSNFHEYVXM3XOJMDS674JZ',
     Claimant.predicateBeforeAbsoluteTime("4102444800000")
  )
];

const op = Operation.createClaimableBalance({
  asset,
  amount,
  claimants
});
  • Add 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.
const op = Operation.createClaimableBalance({
  balanceId: '00000000da0d57da7d4850e7fc10d2a9d0ebc731f7afb40574c03395b17d49149b91f5be',
});

@abuiles abuiles changed the title Add Operation helper to create claimable balances. Add helpers to create CAP23 operations. Sep 23, 2020
@abuiles abuiles requested review from 2opremio, Shaptic, tamirms and a team September 24, 2020 14:41
Copy link
Contributor

@Shaptic Shaptic left a 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) {
Copy link
Contributor

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.

Copy link
Contributor Author

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',
Copy link
Contributor

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)) {
Copy link
Contributor

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?

Copy link
Contributor Author

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)

@abuiles abuiles merged commit c8695d1 into master Sep 24, 2020
@abuiles abuiles deleted the cap-23-ops branch September 24, 2020 21:09
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.

2 participants