Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
CLI command to list tBTC deposits #3545
CLI command to list tBTC deposits #3545
Changes from 2 commits
3189da3
f933ecb
82b381c
3e13eec
fc1cf3a
4d28143
ebac502
6e4e90a
2d10093
3f4e2b3
a717cc2
a52261a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can we rename this command to
list-wallet-deposits
? This will make it more specific.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.
I wanted to keep the command name as short and simple as possible.
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.
I meant something similar to what I mentioned in #3545 (comment). In this case,
deposits
are also ubiquitous, and making it at leastlist-deposits
would make the command clear at first sight, even without referring to its description.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.
There will be just two commands: one to list the deposits and another to submit a deposit sweep proposal. The commands will be used by a handful of development team members, so I don't think there will be any ambiguity in the property naming, compared to the fact that the right transactions and fees will have to be determined by the caller of the sweep proposal command.
The reason why I want to keep all the commands and flags short is that if we combine them with arguments we will get long lines to handle in the CLI, e.g.:
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.
Eventually, the wallet coordinator will manage redemptions, moving funds, and sweeps of moved funds. Most likely, the CLI will have to handle them as well. This is why I'm flagging the need to establish a clear and consistent API of the CLI from the beginning. I'm afraid that we won't be able to keep them short in the long term so it seems that it's better to use a predictable pattern from the beginning. For example, sweeping refers to both deposits and moved funds so we can't just have a single 'sweep' command.
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.
I would also vote for
list-deposits
. We will have more actions in the coordinator soon andlist-deposits
is in my opinion short enough and unique to not confuse it with anything else.The actions we'll have to implement in the coordinator:
... then all the actions related to moving funds. I think it would also be nice to allow submitting heartbeat requests via the coordinator CLI.
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.
Discussion continued in #3549 (comment)
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.
Did you manage to test it on mainnet with the oldest wallet? Did you try calling it against geth server which is not Infura/Alchemy? If not, can you please perform such a test? We had a discussion with @lukasz-zimnoch on how long it takes to retrieve old events without passing a block range. I expect Alchemy and Infura may have some fancier cache/indexing but does it work fast enough with a standard geth? Knowing this would help us determine how fancy we need to be with calls like that and this simple CLI command is a perfect opportunity.
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.
Get All Deposits (no filter):
Mainnet:
Alchemy: 0.205702 seconds
Infura: 0.523039 seconds
Geth: 265.665695 seconds
Get Wallet's Deposits (with wallet filter):
Mainnet:
Alchemy: 0.186728 seconds
Infura: 0.271587 seconds
Geth: 114.994290 seconds
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.
As expected but reassuring our design decisions were correct. Thanks.