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

Update PayByContract to return 'false' if a contract has reached the max revision number #120

Closed
wants to merge 1 commit into from

Conversation

ChrisSchinnerl
Copy link
Member

Not sure if this is the right place to put this considering that the return type is a boolean, but some kind of safeguard against this seems appropriate.

@lukechampine
Copy link
Member

Having false represent multiple distinct types of failure is suspicious. Case in point, it looks like renterd currently does:

        payment, ok := rhpv3.PayByContract(rev, amount, refundAcct, sk)
        if !ok {
                return rhpv3.PayByContractRequest{}, ErrInsufficientFunds
        }

which would be incorrect behavior if !ok can mean "contract not revisable." So this should probably return an error instead.

The other option would be to match the API of PayByEphemeralAccount: instead of failing, return a revision that the host will (presumably) reject. But I think it's better to catch errors locally when possible.

@lukechampine
Copy link
Member

@ChrisSchinnerl are you still interested in merging this, or is renterd using a different approach now?

@lukechampine lukechampine deleted the chris/paybycontract-revnum branch December 4, 2023 22:23
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.

None yet

2 participants