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

account+rpc: allow expiration update for existing accounts #185

Closed
wants to merge 9 commits into from
Closed

account+rpc: allow expiration update for existing accounts #185

wants to merge 9 commits into from

Conversation

wpaulino
Copy link
Contributor

@wpaulino wpaulino commented Dec 2, 2020

This PR allows traders to update their account expiration after it has been created through the RPC/CLI interface. It also serves as groundwork for when we eventually allow automatic account renewals through batch participation. As a result of this functionality, a default of 30 days has been set going forward for all new accounts created, and can be overwritten if necessary.

Fixes #8.
Fixes #39.
Fixes #160.

Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

Cool, this will just work with the existing ModifyAccount call on the auctioneer?

Not many comments, great PR 👍

account/manager.go Outdated Show resolved Hide resolved
cmd/pool/account.go Show resolved Hide resolved
cmd/pool/account.go Show resolved Hide resolved
cmd/pool/account.go Outdated Show resolved Hide resolved
account/watcher/watcher.go Show resolved Hide resolved
account/watcher/watcher_test.go Show resolved Hide resolved
account/manager.go Outdated Show resolved Hide resolved
account/manager.go Outdated Show resolved Hide resolved
poolrpc/trader.proto Outdated Show resolved Hide resolved
cmd/pool/account.go Outdated Show resolved Hide resolved
We no longer need to track if the account expiry needs to be watched or
not as the watcher now supports updating existing unfulfilled expiry
requests.
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Latest version looking pretty good, my comment re allowing while pending batch still stands

account/manager.go Outdated Show resolved Hide resolved
account/manager.go Show resolved Hide resolved
account/manager.go Show resolved Hide resolved
account/manager.go Outdated Show resolved Hide resolved
account/manager.go Outdated Show resolved Hide resolved
poolrpc/trader.proto Show resolved Hide resolved
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🍀

account/manager.go Outdated Show resolved Hide resolved
account/manager.go Outdated Show resolved Hide resolved
This is an intermediate state where an account has met its expiration
while having a pending update waiting for confirmation. The introduction
of this state was necessary as the existing StateExpired state was not
sufficient to handle all edge cases regarding account renewals. Now
that this state exists, we can be sure renewals cannot happen until the
account is both confirmed, unspent, and expired.
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

Great work, LGTM ✅

account/manager.go Show resolved Hide resolved
account/manager.go Show resolved Hide resolved
@guggero
Copy link
Member

guggero commented Jan 12, 2021

Replaced by #199.

@guggero guggero closed this Jan 12, 2021
@wpaulino wpaulino deleted the account-renewal branch January 12, 2021 18:55
Roasbeef added a commit that referenced this pull request Jan 14, 2021
Fix lnd v0.12.0-beta compatibility, combine #142 and #185
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.

accounts: default value for expiration Create account expiry field is vague accounts: renew option
4 participants