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 htlc api calls #1729

Closed
wants to merge 1 commit into from
Closed

Conversation

oxarbitrage
Copy link
Member

@oxarbitrage oxarbitrage commented Apr 22, 2019

Note: changes in this pull requests have been merged in #1742.

partially resolves #1713 by adding 3 api calls:

  • get_htlc(id)
  • get_htlc_by_from(account, start, limit)
  • get_htlc_by_to(account, start, limit)

The get_htlc_by_from and get_htlc_by_to follows some of the last group of api calls added for the recurring payments(get_withdraw_permissions_by_giver and get_withdraw_permissions_by_recipient)

  • htlcs objects were added to full accounts, only the from ones following again the withdraw calls. The to ones can be added easily if needed.

Unsure if we really need a call to list all htlc objects in the blockchain, maybe a use case will help here.

No cli wallet commands are added in this pull request, i tried to make it small enough to be able to get in the next feature release.

@abitmore abitmore added this to the 3.1.0 - Feature Release milestone Apr 22, 2019
@sschiessl-bcp
Copy link

  • I'd stick to the same wording for the calls (giver and recipient).
  • Call for all HTLC? Use case that comes to mind is monitoring, but not important IMO (need for an off-chain HTLC order book exists anyways)
  • get_htlc_by_hash would come in handy (since hash is what people normally exchange)

std::for_each(htlc_range.first, htlc_range.second,
[&acnt] (const htlc_object& htlc) {
acnt.htlcs.emplace_back(htlc);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Also list htlcs for which the account is the recipient?


vector<htlc_object> database_api_impl::get_htlc_by_from(const std::string account_id_or_name, htlc_id_type start, uint32_t limit) const
{
FC_ASSERT( limit <= 101 );
Copy link
Contributor

Choose a reason for hiding this comment

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

When adding new API calls with limits, please make the limit configurable as in #1513

vector<htlc_object> database_api_impl::get_htlc_by_to(const std::string account_id_or_name, htlc_id_type start, uint32_t limit) const
{

FC_ASSERT( limit <= 101 );
Copy link
Contributor

Choose a reason for hiding this comment

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

When adding new API calls with limits, please make the limit configurable as in #1513


/**
* @brief Get HTLC object
* @param id HTLC contarct id
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: contarct

/**
* @brief Get non expired HTLC objects using the sender account
* @param account Account ID or name to get objects from
* @param start Withdraw permission objects(1.16.X) before this ID will be skipped in results. Pagination purposes.
Copy link
Contributor

Choose a reason for hiding this comment

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

C&P error

/**
* @brief Get non expired HTLC objects using the receiver account
* @param account Account ID or name to get objects from
* @param start Withdraw permission objects(1.16.X) before this ID will be skipped in results. Pagination purposes.
Copy link
Contributor

Choose a reason for hiding this comment

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

C&P error

@jmjatlanta jmjatlanta mentioned this pull request Apr 29, 2019
@pmconrad
Copy link
Contributor

Closing in favour of #1742

@pmconrad pmconrad closed this Apr 30, 2019
@pmconrad pmconrad removed this from the 3.1.0 - Feature Release milestone Apr 30, 2019
@syalon
Copy link
Member

syalon commented May 1, 2019

does full accounts now contain htlc objects for to ones?

@abitmore
Copy link
Member

abitmore commented May 1, 2019

@syalon yes.

@abitmore
Copy link
Member

abitmore commented May 1, 2019

@pmconrad this PR would be marked as "merged" automatically if you didn't close it (move the mouse to the "reopen and comment" button then we can see a tooltip).

@abitmore abitmore added this to the 3.1.0 - Feature Release milestone May 1, 2019
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.

5 participants