-
Notifications
You must be signed in to change notification settings - Fork 37
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
fix: various faucet fixes for next #362
Conversation
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.
LGTM! I think it would be good if we could add two changes:
- If the spinner is active, make sure the buttons are deactivated/don't do anything when clicked in order to avoid people spamming requests.
- Judging by the video it looks like the spinner moves the page layout a bit which is probably not ideal.
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.
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 left some small comments. I'm waiting for the docker image to build so I can test it 😅
bin/faucet/src/handlers.rs
Outdated
utils::{build_client, FaucetState}, | ||
}; | ||
|
||
const TOKEN_AMOUNT_OPTIONS: [u64; 3] = [100, 500, 1000]; |
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.
wasn't this already part of the config? I can still see it in miden-faucet.toml
bin/faucet/src/handlers.rs
Outdated
@@ -23,14 +29,14 @@ struct FaucetRequest { | |||
#[derive(Serialize)] | |||
struct FaucetMetadataReponse { | |||
id: String, | |||
asset_amount_options: Vec<u64>, | |||
asset_amount_options: [u64; 3], |
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.
why do we need the change to a fixed size array?
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.
This was one of the changes I left on my PR to main
, but I think @tomyrd is going to roll it back into using the configuration field.
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.
Done.
Co-authored-by: igamigo <ignacio.amigo@lambdaclass.com>
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.
Tested and LGTM.
Only thing I could say is that I believe that you should run cargo update
and commit before merging because we are now also tracking the cargo.lock file.
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.
Looks good! Thank you!
This PR fixes a few things related to the faucet:
main
tonext
.Here is a video showing the new spinner:
loading_spinner.mov