Skip to content
This repository has been archived by the owner on Jan 11, 2024. It is now read-only.

Fix prefund #405

Closed
wants to merge 1 commit into from
Closed

Fix prefund #405

wants to merge 1 commit into from

Conversation

cryptoAtwill
Copy link
Contributor

@cryptoAtwill cryptoAtwill commented Nov 29, 2023

Fix prefund by calling fund if prefund fails.

This should fix consensus-shipyard/ipc#353 and consensus-shipyard/ipc#357

Copy link

Your PR was set to target main. PRs should be target dev.
The base branch of this PR has been automatically changed to dev.
If you really intend to target main, edit the PR.

@github-actions github-actions bot changed the base branch from main to dev November 29, 2023 05:03
Copy link
Contributor

@adlrocha adlrocha left a comment

Choose a reason for hiding this comment

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

I don't think the current implementation fully solve the issues linked, would you mind having a look at my comments? Thanks!

.pre_fund(subnet.clone(), from, f64_to_token_amount(initial_balance)?)
.await?;
if initial_balance.is_zero() {
return Err(anyhow!("not enough balance"));
Copy link
Contributor

Choose a reason for hiding this comment

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

This check is already being done in the contract, so this one is redundant (in case you want to remove this check): https://github.com/consensus-shipyard/ipc-solidity-actors/blob/9c85f9d110ba10737d258094085e70bb9f0ad54d/src/subnet/SubnetActorManagerFacet.sol#L97

return Err(anyhow!("not enough balance"));
}
let amount = f64_to_token_amount(initial_balance)?;
if let Err(e) = provider
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this fully solves the issues that you linked:

  • For the atomicity of prefund + join. It may be the case that a join has failed but the prefund transaction went through, which means that when calling the command again, additional initial-balance will be passed when the user actually wanted the original initial-balance. The right way of addressing this should be to:
    • Check if the validator already has an initial balance in the contract.
    • If no, then execute pre-fund normally and then call join.
    • If yes, then check if the initial balance is equal to the amount passed in --intial-balance. If it is the same, do not call pre-fund and call join. If the initial balance is not equal to --initial-balance then call pre-fund for the difference in initial balance requested for the user.
  • On the other hand, for the pre-fund/fund difference, we can simplify the logic by:
    • If the subnet is already bootstrapped, calling first join, and then calling fund. That way, if join failed, fund will never be called and we won't have the atomicity problem that we had above.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants