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

Don't put new chains in wallet if we don't own them. #2725

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

afck
Copy link
Contributor

@afck afck commented Oct 27, 2024

Motivation

The long chain faucet test fails (os error 16 for me) unless we explicitly forget the opened chains.

It looks like we can't use a faucet with a wallet with 1000 chains in it fails. (I suspect the same would apply to the node service.) This is something we should look into as well.

Proposal

Make the open-chain CLI command not add new chains to the wallet if we don't own them.

Keeping this as a draft for now:

  • It's maybe debatable what the desired behavior for the CLI command is.
  • The node service mutation doesn't seem to ever add the new chain to the wallet? (Will look into this and create an issue on Monday.)
  • We should investigate why the faucet fails if it has 1000 chains in the wallet.

Test Plan

I removed the forget_chain from the test and it still passes now.

Release Plan

  • These changes should be backported to the latest devnet branch, then
    • be released in a new SDK.
  • These changes should be backported to the latest testnet branch, then
    • be released in a new SDK.

Links

@afck afck requested review from ma2bd, jvff and christos-h October 27, 2024 11:10
.update_wallet_for_new_chain(id, key_pair, timestamp)
.await?;
if key_pair.is_some() {
context
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add a warning and suggest a command to add the chain later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We currently don't have such a command, I think. assign only works if you do have the key. We could add an add-chain command that works like assign but without a key? But then maybe add-chain should replace assign, and the key should be an optional parameter?

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.

Try to use a throwaway key in long faucet chain test
2 participants