-
Notifications
You must be signed in to change notification settings - Fork 75
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
How to set fields on Transactions #1643
Comments
Originally posted by @leighmcculloch in #1702 (comment) |
Fleshing out the idea I alluded to above here's one idea to start the design process. I think given the tx structure is a tree of related fields and sub-fields, we could organise this transaction command the same.
Some of these subcommands are pretty long, e.g. Preconditions actually have multiple ways to be constructed when there are no preconditions, and when there are only time bounds, and I think the CLI can just optimise around that, the UI doesn't need to expose those differences. The CLI can upgrade and downgrade the conditions as necessary to keep the tx as small as possible. Thoughts? cc @sagpatil @fnando @willemneal |
Is it necessary to follow the commands from a tree of related fields or could we drop words like |
@janewang Nothing is set in stone, we could drop the tree structure, which is similar to what @willemneal did in his PR, although the PR is options oriented rather than sub-commands. The challenge with dropping no hierarchy is that some modifications of the tx have ambiguous scope. If we get rid of the |
@janewang are you suggesting something such as:
Or something such as:
@willemneal If we ignore the "preconditions none" / "--no-preconditions" case, it does seem much less ambiguous. The only option that seems a little odd at that point is the extra signers one. |
My 2 cents: it'd be easier to chain tx edit with flags, and I think it's a very common use case to setup multiple things at once. ( |
For input to whether we need flags for some of the advanced options, according to the stellar hubble bigquery dataset there is some usage of min-account-sequence, and rare usage of others.
SELECT
EXTRACT(YEAR from created_at),
COUNTIF(min_account_sequence IS NOT NULL AND min_account_sequence != 0) as min_account_sequence,
COUNTIF(min_account_sequence_age IS NOT NULL AND min_account_sequence_age != 0) as min_account_sequence_age,
COUNTIF(min_account_sequence_ledger_gap IS NOT NULL AND min_account_sequence_ledger_gap != 0) as min_account_sequence_ledger_gap,
COUNTIF(extra_signers IS NOT NULL AND ARRAY_LENGTH(extra_signers) != 0) as extra_signers
FROM `crypto-stellar.crypto_stellar.history_transactions`
GROUP BY 1
ORDER BY 1 DESC
LIMIT 1000 |
For the extra-signers functionality, I think it's worth keeping that outside the But it's also such a low used feature, it could be argued not worth adding the command for. But if it's easy enough may as well. |
🤔 I have another idea for us to consider: flip the
The above would pave the way for a rich set of future commands:
Automatically resets it based on the source account, setting it to the next valid value.
Auto set seq num based on 5 more than the source account currently has:
Auto set time bounds based on 5 mins from now:
Using this model loses the convenience @Ifropc mentioned about being able to combine multiple together, but it paves the way for a rich set of commands for getting at fields, not just setting them. |
If we did the above, I think we'd need to move the subcommand under |
While I was happy for the solution with minimal changes, I think the second one is must cleaner and will make it clearer to new devs. This does come at the cost of terseness as @Ifropc, points out will make setting multiple fields more verbose. There is a solution for example @Ifropc @janewang If there is agreement I can start on @leighmcculloch's last suggestion, |
-- @willemneal #1785 (comment) This would be really convenient. I think we could in addition provide the same functionality in a way that is composable. For example:
|
Originally posted by @leighmcculloch in #1551 (comment)
cc @willemneal
The text was updated successfully, but these errors were encountered: