-
Notifications
You must be signed in to change notification settings - Fork 36
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
Improve flow for guarded accounts #495
Conversation
multiversx_sdk_cli/cli_shared.py
Outdated
default="", | ||
) | ||
sub.add_argument("--relayer", type=str, help="the bech32 address of the relayer", default="") | ||
sub.add_argument("--guardian", type=str, help="the bech32 address of the guradian", default="") |
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.
typo, guradian
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.
fixed
try: | ||
return trusted_cosigner_service_url_by_chain_id[chain_id] | ||
except: | ||
raise BadUsage(f"Could not get Trusted Cosigner Service Url. No match found for chain id: {chain_id}") |
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.
Will this fail when using mxpy for testing, in the context of internal testnets? There, chain ID is just like on mainnet (but the cosigner service has another URL, of course).
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.
Yes, this will fail. We should think of another solution if we want this to work with the internal testnets. The user can still provide all guardian related arguments.
network_provider_config = config.get_config_for_network_providers() | ||
proxy = ProxyNetworkProvider(url=proxy_url, config=network_provider_config) | ||
|
||
response = proxy.do_get_generic(f"/address/{address}/guardian-data").to_dictionary() |
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.
Oh, now I see, we don't have this in the specs / network provider. We only define AccountOnNetwork.is_guarded
:
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.
Maybe we should capture this in the specs, since it's relatively important endpoint (not right now, of course)?
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.
We can think about it, but sure, it can be added.
multiversx_sdk_cli/cli_shared.py
Outdated
return guardian_data | ||
|
||
|
||
def _get_2fa_code(args: Any) -> str: |
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.
Maybe rename to _ask_2fa_code
(to stand out from the get*
and fetch*
functions from this module).
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 see we have a file for asking passwords (just a note, not a recommendation):
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.
renamed to _ask_for_2fa_code
.
guardian_and_relayer_data.guardian_2fa_code = _get_2fa_code(args) | ||
|
||
|
||
def _get_guardian_data(address: str, proxy_url: str) -> Union[dict[str, str], None]: |
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.
Just a reference for double checking / no additional action to take:
active_guardian = guardian_data.get("activeGuardian", {}) | ||
|
||
guardian_address = active_guardian.get("address", "") | ||
service_id = active_guardian.get("serviceUID", "") |
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.
We should bring this in the SDKs (and specs), I think.
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.
If we decide on it, we can do it.
If the account is guarded using our
Trusted Cosigner Service
there is no need to provide the--guardian
and--guardian-service-url
arguments. That is of course, only when--proxy
is provided. The guardian is fetched from the network and the cosigner service url is known.All the guardian related args can be set from the cli.