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

PLT-6847 withdraw by payout #694

Merged
merged 10 commits into from Aug 28, 2023
Merged

PLT-6847 withdraw by payout #694

merged 10 commits into from Aug 28, 2023

Conversation

jhbertra
Copy link
Contributor

@jhbertra jhbertra commented Aug 22, 2023

  • Withdraw now accepts payout tx out refs instead of contact ID + token name
  • all tests and clients updated
  • REST API updated
  • Payout schema changed to align with other entities and provide more consistent interface (thanks @nhenin)
    • PayoutRef renamed to PayoutHeader
    • Consistent use of payoutId vs payout
    • use AssetId for role field instead of two fields
    • use more clear terminology and an explicit status field for withdrawn / available payout filtering.

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Test report is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Relevant tickets are mentioned in commit messages
    • Formatting, PNG optimization, etc. are updated
  • PR
    • Self-reviewed the diff
    • Useful pull request description
      • Review required
      • Substantial changes to code, test, or documentation
      • Change made to Marlowe validator (@bwbush and @palas must be included as reviewers)
      • Review not required
      • Minor changes to non-critical code, documentation, nix derivations, configuration files, or scripts
      • Formatting, spelling, grammar, or reorganization
    • Reviewer requested

@jhbertra jhbertra self-assigned this Aug 22, 2023
Comment on lines +119 to +124
WithdrawConstraintError err -> ApiError (show err) "ConstraintError" Null case err of
MintingUtxoNotFound _ -> 400
RoleTokenNotFound _ -> 400
PayoutNotFound _ -> 400
CoinSelectionFailed _ -> 400
_ -> 500
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this change make error reporting less expressive? The error reports were already too terse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't make it less expressive, it actually makes it more expressive:

-- before on "PayoutInputNotFound"
ApiError "Internal error" "PayoutInputNotFound" Null 500

-- after
ApiError "PayoutNotFound <payout tx in>" "ConstraintError" 500

Comment on lines +3 to +5
- BREAKING `Withdraw` now accepts a set of payout tx outs instead of a contract
ID and a role token. The old behaviour can be emulated via a query to fetch
unclaimed payouts for a contract.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll have to revise some starter-kit lessons prior to releasing this.

Copy link
Collaborator

@bwbush bwbush 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, but I haven't done ad-hoc testing on chain. I assume that @nhenin will do that as he uses the new API.

@nhenin
Copy link
Contributor

nhenin commented Aug 27, 2023

@jhbertra @bwbush , I have finished the integration on the ts-sdk and added a couple of fixes at the end of your commits @jhbertra.

We can merge it imo.

@jhbertra jhbertra merged commit d2352eb into main Aug 28, 2023
5 of 6 checks passed
@jhbertra jhbertra deleted the plt-6847-withdraw-by-payout branch August 28, 2023 16:03
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

3 participants