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

Signature deposit #611

Merged
merged 9 commits into from
Jun 6, 2024
Merged

Signature deposit #611

merged 9 commits into from
Jun 6, 2024

Conversation

volovyks
Copy link
Collaborator

We will need to add a refund functionality after wee implement yield/resume approach.

This was linked to issues May 23, 2024
@volovyks volovyks requested review from ppca and ChaoticTempest and removed request for ppca May 23, 2024 23:27
@volovyks volovyks marked this pull request as ready for review May 23, 2024 23:27
Copy link
Member

@ChaoticTempest ChaoticTempest left a comment

Choose a reason for hiding this comment

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

A test here with relayer making a call into the contract would also be nice to see just in case relayer cannot fund the request

@volovyks
Copy link
Collaborator Author

@ChaoticTempest we do not have Relayer setup in multichain test and I do not think we need it just to test if deposits work with relayer. But I will doublecheck if it works before we merge this.

@volovyks volovyks requested a review from ChaoticTempest May 29, 2024 15:53
@volovyks
Copy link
Collaborator Author

I had a call with @r-near. We should be fine with passing DelagateActions with deposits.

/// To avoid overloading the network with too many requests,
/// we ask for a small deposit for each signature request.
/// The fee changes based on how busy the network is.
pub fn signature_deposit(&self) -> u128 {
Copy link
Member

Choose a reason for hiding this comment

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

now that I think about this a bit more, I'm not sure if allowing people to call this function to check what's the required deposit would be great experience, since this amount can change in the time they call this function and when they call sign. This would cause a lot of retries which could further congest the network.

So instead, to avoid having this situation, we should ask the users to always make the highest deposit we can take when calling into sign, and then we can refund the difference to the account if the cheaper requests are possible

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

People may want to spend 1 yN maximum.
If they provide more than we need, we will definitely refund (separate PR).
You may be right that it should not be a part of the API since it will work in ~90% of cases which is not enough. People can call sign() with the maximum amount they are ready to spend and get an error if the network is under load.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'm also think people would always deposit the maximum deposit to ensure it get processed. And we can refund the difference based on load

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we will add refund logic after yield/resume is merged

ChaoticTempest
ChaoticTempest previously approved these changes Jun 3, 2024
ailisp
ailisp previously approved these changes Jun 4, 2024
@volovyks volovyks dismissed stale reviews from ailisp and ChaoticTempest via 4ba8474 June 4, 2024 17:36
@volovyks volovyks requested review from ChaoticTempest and ailisp June 4, 2024 17:37
@volovyks volovyks merged commit a90a991 into develop Jun 6, 2024
6 checks passed
@volovyks volovyks deleted the serhii/payable-sign branch June 6, 2024 13:41
Copy link

github-actions bot commented Jun 6, 2024

Terraform Feature Environment Destroy (dev-611)

Terraform Initialization ⚙️success

Terraform Destroy success

Show Destroy Plan


No changes. No objects need to be destroyed.

Either you have not created any objects yet or the existing objects were
already deleted outside of Terraform.

Destroy complete! Resources: 0 destroyed.

Pusher: @volovyks, Action: pull_request, Working Directory: ``, Workflow: Terraform Feature Env (Destroy)

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.

Require an attached deposit of 1 yocto for sign Paid requests
3 participants