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

Faucet API #1

Merged
merged 10 commits into from
Nov 21, 2023
Merged

Faucet API #1

merged 10 commits into from
Nov 21, 2023

Conversation

gianfra-t
Copy link
Contributor

This PR correspond to the first iteration of the faucet service API.

Please refer to the README for explanation about what the service does.

@gianfra-t gianfra-t linked an issue Nov 17, 2023 that may be closed by this pull request
@gianfra-t gianfra-t requested a review from ebma November 17, 2023 13:16
Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

Great job @gianfra-t 👍 I left minor comments but it looks good overall

src/chain_service/faucet.ts Outdated Show resolved Hide resolved
src/server.ts Show resolved Hide resolved
src/chain_service/faucet.ts Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/slack_service/slack.ts Outdated Show resolved Hide resolved
src/chain_service/faucet.ts Outdated Show resolved Hide resolved
return reject(`Service Error: Invalid Mnemonic`);
}

const nonce = await api.api.rpc.system.accountNextIndex(sender.publicKey);
Copy link
Member

Choose a reason for hiding this comment

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

Are we actually able to support multiple incoming faucet requests by different users or would this nonce here be a dealbreaker? Maybe it works perfectly fine, just wondering

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested it by sending multiple requests at the "same time" (manually) and they all worked, or at least entered in the same block.
What is attempted to do here by fetching the nonce is exactly that, to allow multiple fund events simultaneously.

Now I wonder if the nonce is able to increase if the transactions arrive faster that what could manually be tested. I guess that to be on the safe side, we could implement a mutex similar to what we did here. The problem I see with something like that is that it could delay a lot the completion. WDYT?

Copy link
Member

@ebma ebma Nov 21, 2023

Choose a reason for hiding this comment

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

Ah nice, I was not fully aware how the accountNextIndex works. But since this apparently is taking the pending transactions of the transaction pool of the node into account (see here), I think it's fine and should indeed work as expected.

I already noticed that the Spacewalk testing service was able to submit multiple extrinsics in the same block so I can confirm that this works. Let's not add a mutex as it's not necessary then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think it is also important for that to keep the await in cases like in the example to ensure that the transaction reached the pool.
It is still not clear to me what would happen if 2 transactions were to be sent in which the first transaction does not reach the pool before the second one is sent.

Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

Looks good to me and ready for merge 👍

@gianfra-t gianfra-t merged commit 9596007 into main Nov 21, 2023
@ebma ebma deleted the first-iteration branch November 22, 2023 09:44
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.

Add faucet to send test tokens to accounts
2 participants