-
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
Implemented a native auth client and faucet functionality #418
Conversation
Dockerfile
Outdated
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.
what is the usage of this docker image?
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.
it was committed by accident. removed!
api_url: str = "https://api.multiversx.com", | ||
expiry_seconds: int = 60 * 60 * 24, |
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 set constants for these default values?
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.
made some constants for that values
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.
here I was referring for these 2 variables: default api url and expiry time in seconds; i think it can be only one var for expiry_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.
indeed, makes sense. done!
cli_shared.add_wallet_args(args, sub) | ||
sub.add_argument("--chain", required=True, help="the chain identifier") |
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.
here it lists --pem, --keyfile, --ledger, --chain
as required, but only one of the first three are required, right?
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.
one of --pem, --keyfile, --ledger
is required, but also the --chain
is required to specify on which network you wish to receive the funds
|
||
def call_web_Wallet_faucet(wallet_url: str, access_token: str): | ||
faucet_url = f"{wallet_url}/faucet?accessToken={access_token}" | ||
webbrowser.open_new_tab(faucet_url) |
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'm thinking of the main benefits of having this command from cli, since we still have to check in the browser
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've had people asking about it and i think it's a little bit easier to use the cli instead of doing it manually even though you have to pass the recaptcha and then press the request button. This is just an initial implementation/poc. We were thinking about doing it another way, so the user does not have to do anything else but type the command but we have not yet found a non-exploitable way to do it and we'll also need some help from the colleagues working on the api.
NativeAuthClientConfig) | ||
|
||
|
||
def mock(mocker: Any, code: int, response: Any): |
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.
Nice 🚀
assert access_token == self.ACCESS_TOKEN | ||
|
||
|
||
class TestNativeAuthWithGateway: |
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.
Both cases, nice coverage of flows 🙌
|
||
from multiversx_sdk_cli.errors import NativeAuthClientError | ||
|
||
EXPIRY_TIME_IN_SECONDS = 60 * 60 * 2 |
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 also have prefix 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.
added
response = self._execute_request(url) | ||
return response[0]["hash"] | ||
|
||
def encode_value(self, string: str) -> 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.
Does this help?
https://docs.python.org/3/library/base64.html#base64.urlsafe_b64encode
=
would have to be trimmed, nonetheless.
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.
Might help, but indeed =
would have to be trimmed as well. Will leave it as it is for now.
def __init__(self, config: Optional[NativeAuthClientConfig] = None) -> None: | ||
self.config = config or NativeAuthClientConfig() | ||
|
||
def get_token(self, address: str, token: str, signature: str) -> 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.
Functions can be re-ordered to match the usual flow. E.g. first "initialize", then ...
url = f"{self.config.gateway_url}/blocks/by-round/{round}" | ||
response = self._execute_request(url) | ||
blocks = response["data"]["blocks"] | ||
block = [b for b in blocks if b["shard"] == self.config.block_hash_shard][0] |
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.
A bit fragile - it's possible to have a round where not all shards propose a block (missed block).
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.
However, in the context of mxpy, we don't provide the shard.
Use
mxpy faucet request
to test the faucet functionality. It also requires a wallet and the chain id of the network you wish to receive funds on.