-
Notifications
You must be signed in to change notification settings - Fork 220
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
Creating faucet for jormungandr #424
Conversation
- address: ta1s07wt2t29f6jum5lhqvddj6rxglrcpexh7xv49jdkrl67yzx0pdzzc3wnmv | ||
value: 100000000000 | ||
- address: ta1sv75hznxzw783x5s44llz49x5xx4nsasnjrw6wuj4h505vkhqeq0uq77d56 | ||
value: 100000000000 |
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 suppose these addresses comes from 10 different wallets (would be nice to put as a comment).
Note that, for the integration tests faucets to be of similar shape with the one used with the bridge, we need each wallet to have 10 UTxO available (this allows for testing things like multi-output transactions and coin selection more easily). There's no issue in re-using the same address for that, so each line here should be duplicated 10x times (modulo the amount which also could deserve a comment for readability :))
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 changed the values 10x to accommodate up to 10 UTxOs per wallet each having 10^12 plus added corresponding comment. And regenerated block-0.bin
(["where","circle","abstract","estate","bless","state","stool","future","beauty","episode","patient","rhythm","scare","project","trigger"], | ||
"\131=K\138f\DC3\188x\154\144\173\DEL\241T\166\161\141Y\195\176\156\134\237;\146\173\232\250\&2\215\ACK@\254", | ||
"ta1sv75hznxzw783x5s44llz49x5xx4nsasnjrw6wuj4h505vkhqeq0uq77d56")] | ||
--} |
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 to have the code used for generation here! I don't think we need to have the full ghci excerpt though, since the output is already hard-coded above.
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.
cut considerably ghci excerpt, just leaving ghci> generateTriples 1
output to have automatically what to invoke when needed
-- | Initialize a bunch of faucet wallets and make them available for the | ||
-- integration tests scenarios. | ||
initFaucet :: NetworkLayer t IO -> IO Faucet | ||
initFaucet _nl = do |
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's this NetworkLayer
argument here for 🤔 ?
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.
no needed at this moment, hence I removed it
putAddress addr = do | ||
-- NOTE: only single address supported for now | ||
putWord8 (single @n) | ||
putByteString addr |
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.
As discussed on @Anviking's PR, it would be best to keep the binary stuff inside the Binary module and move putAddress
there 👍
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 can be here for the time being, but I think my PR should be merged soon fixing this.
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 is temporary as mentioned in the bullet points of description, I will wait for @Anviking PR to be merged into master and will take the fix as it is
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!
2e6ba01
to
0488c61
Compare
# For the integration test's faucets to be of similar shape | ||
# with the one used with the http-bridge, | ||
# we need each wallet to have 10 UTxO available, hence the value for each | ||
# wallet is 10x bigger than what is ascribed to each UTxO. |
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.
hence the value for each wallet is 10x bigger than what is ascribed to each UTxO.
Making the value bigger doesn't make more UTxO available 😉 UTxOs are like bank notes, here we don't want a 500 EUR note, but we do want 10 x 50 EUR notes! So, instead of
- address: ta1svz5hw3leda47fy3scdku2ut7z3ljftumuha7sw2ttmswtnhcu8n5nm0ynp
value: 1000000000000
We should have
- address: ta1svz5hw3leda47fy3scdku2ut7z3ljftumuha7sw2ttmswtnhcu8n5nm0ynp
value: 100000000000
- address: ta1svz5hw3leda47fy3scdku2ut7z3ljftumuha7sw2ttmswtnhcu8n5nm0ynp
value: 100000000000
.
. ( x 10 in total)
.
- address: ta1svz5hw3leda47fy3scdku2ut7z3ljftumuha7sw2ttmswtnhcu8n5nm0ynp
value: 100000000000
And this for each address!
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 in: 94ff494
let wallets = | ||
map | ||
((either (error "cannot retrieve mnemonics") id . mkMnemonic @15) . fst) | ||
mnemonicsAdresses |
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.
minor style suggestion, it reads better to define an intermediate function for this kind of things, like
let wallets = unsafeMkMnemonic @15 . fst <$> mnemonicsAddresses
22570ee
to
c60f49c
Compare
replaced with an intermediate function to also ease readability
c60f49c
to
e8781ec
Compare
Issue Number
#358
Overview
Comments