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

feat: add redeeming to LP API #4292

Merged
merged 2 commits into from Dec 5, 2023
Merged

feat: add redeeming to LP API #4292

merged 2 commits into from Dec 5, 2023

Conversation

j4m1ef0rd
Copy link
Contributor

@j4m1ef0rd j4m1ef0rd commented Nov 30, 2023

Pull Request

Closes: PRO-959

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have updated documentation where appropriate.

Summary

TODO: Add new command to the docs

  • Didn't add this command to the bouncer test. Would be quite a bit of work to do the full redeem flow or reuse any of the fundRedem test code. Not sure if it's worth the time.

Sample usage, redeem all:

curl -H "Content-Type: application/json" -d '{"id":1, "jsonrpc":"2.0", "method": "lp_request_redemption", "params": ["0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266", null, "0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266"]}' http://localhost:10589

@j4m1ef0rd j4m1ef0rd self-assigned this Nov 30, 2023
Copy link
Collaborator

@dandanlen dandanlen left a comment

Choose a reason for hiding this comment

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

I think if we want to keep the interface similar to the cli, it should be something like:

async fn request_redemption(
		&self,
		redeem_address: EthereumAddress,
		exact_amount: Option<NumberOrHex>,
		executor_address: Option<EthereumAddress>,
	) -> Result<Hash, AnyhowRpcError>;

This way the query is { redeem_address: "0xabab.." } for a max redeem, and { redeem_address: "0xabab..", exact_amount: "0xb1ab1a.." } for an exact redeem, which is closer to the cli convention of redeem vs redeem --exact 1234.

With the current solution we're introducing a new convention of amount: "Max".

FWIW I don't think the latter is a bad idea necessarily (it's more explicit than assuming max by default) but it should be a conscious decision, and then we should plan to update the CLI too to make things consistent.

api/bin/chainflip-lp-api/src/main.rs Outdated Show resolved Hide resolved
@dandanlen dandanlen merged commit c9a0616 into main Dec 5, 2023
40 checks passed
@dandanlen dandanlen deleted the feat/redeem-in-lp-api branch December 5, 2023 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants