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

keys fund: top-off accounts if below --minimum-balance #1389

Closed
chadoh opened this issue Jun 18, 2024 · 9 comments · Fixed by stellar/go#5399, stellar/quickstart#621, #1501 or #1508
Closed

keys fund: top-off accounts if below --minimum-balance #1389

chadoh opened this issue Jun 18, 2024 · 9 comments · Fixed by stellar/go#5399, stellar/quickstart#621, #1501 or #1508
Assignees

Comments

@chadoh
Copy link
Contributor

chadoh commented Jun 18, 2024

What problem does your feature solve?

Right now if I have an existing funded account, let's call it alice, if I run keys fund alice I will get an error:

$ soroban keys fund alice --network local
Account already exists

What would you like to see?

I have always found this error message a little odd. I know that the account exists! Or rather, it's not important to me whether it exists or not. If the account already exists and the balance is above some number of XLM (maybe 100?), then everything's good. It can tell me that.

$ soroban keys fund alice --network local
Current alice balance: 135 XLM. Nothing to do. 
Set a minimum with: --minimum-balance (default: 100)

If funding occurs:

$ soroban keys fund alice --network local --minimum-balance 200
Current alice balance: 135 XLM. Topping off... 
New alice balance: 1135 XLM.

The maximum --minimum-balance should probably be 1000, since that's what Friendbot dishes out to new accounts.

Friendbot doesn't actually allow funding existing accounts, so behind the scenes this will need to create a new account and transfer all of its balance to the desired account.

What alternatives are there?

We could just not? Make people top off long-lived test accounts themselves, or keep generating new ones.

@leighmcculloch
Copy link
Member

Friendbot doesn't actually allow funding existing accounts, so behind the scenes this will need to create a new account and transfer all of its balance to the desired account.

It requires some discussion, probably in an issue on the stellar/go repo, but we can change friendbot so it better serves developers. I think we should open an issue about funding existing accounts, which wouldn't be difficult (🤞🏻) to add to friendbot.

The codebase for friendbot is here: https://github.com/stellar/go/tree/master/services/friendbot

@leighmcculloch
Copy link
Member

leighmcculloch commented Jul 11, 2024

Similar to @fnando's comment at #1380 (comment) I don't think we should add --minimum-balance and instead just have fund always fund if the balance is below the amount that friendbot will send. i.e. if the balance is below 10000, deposit 10000. Friendbot can do that check. So the fund function makes one guarantee, the bal is at least the amount friendbot funds if successful.

@BlaineHeffron
Copy link
Contributor

My PR adds (10,000 XLM - current_balance). This was because if I just added 10,000 XLM, I would always get an error: topics:[error, Error(Contract, #10)], data:[\"resulting balance is not within the allowed range\", 10000000, 0, 9223372036854775807]

Not sure if that is a bug in the asset contract or if 10k XLM is just intended to be a hard limit. But the workaround was to just top it off up to 10k

@leighmcculloch
Copy link
Member

leighmcculloch commented Jul 24, 2024

I think we should add this functionality to friendbot not the CLI, so more than just the CLI benefit from the change, and to a lesser degree so that funding doesn't use 2x the ledger space and take 10-14 seconds, but a single ledger and 5-7 seconds. The friendbot change is a pretty small change, and I understand it's a bit left field of a contribution to make and ramp up on, so I've gone ahead and opened the change here:

@chadoh
Copy link
Contributor Author

chadoh commented Jul 26, 2024

@leighmcculloch when will stellar/go#5399 be incorporated into quickstart? Deployed to futurenet & testnet? I'd like to keep this issue around until we can try out the CLI with those changes to see if we need to update any CLI logic. I think error handling in our fund_address logic might need to be updated, at a minimum.

@chadoh chadoh reopened this Jul 26, 2024
@leighmcculloch
Copy link
Member

leighmcculloch commented Jul 26, 2024

Good call:

@BlaineHeffron
Copy link
Contributor

Does the CLI utilize the new friendbot functionality or is that something we will need to add in a separate PR? @leighmcculloch

@leighmcculloch
Copy link
Member

leighmcculloch commented Jul 30, 2024

Yup, the cli fund command will top up account that drops below 10k. And errors will be communicated back to the user.

If the user tries to fund when Friendbot won't because its balance is too high the error from Friendbot will propagate to the command line.

It might be a better UX to ignore that error case in the cli and treat it like the success case, since even though Friendbot didn't fund more the account is still funded because it has at least 10k balance.

Thoughts?

@BlaineHeffron
Copy link
Contributor

I agree, I think it should ignore the error if the user already has 10k or more and perhaps just communicate that to the user - "Balance already above 10k, nothing to do" or something like that. Thoughts, @chadoh ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment