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

XRPL Multisig Prover #227

Draft
wants to merge 83 commits into
base: main
Choose a base branch
from

Conversation

abresas
Copy link

@abresas abresas commented Jan 3, 2024

Support for payments (native or with token) using tickets, and updating multisig signers.

  • We do not yet support GMP
  • Payment amounts are passed as cosmwasm coin amount, yet precision loss needs to be discussed
  • We have (temporarily) added an xrpl_voting_verifier contract that allows us to differentiate between different message statuses. The generic voting_verifier has functionality for supporting this, but doesn't expose the actual status of a message yet.
  • We haven't yet implemented rewards which is a new feature that wasn't part of the generic multisig prover when we started the project, and we are not sure if and how they should be implemented.
  • This PR requires custom signature verification support from multisig contract, which isn't yet supported.
  • We have a first implementation of integration tests. However, the integration tests fail, unless signature verification on multisig is disabled (commented out), until custom signature verification is implemented.
  • No support for trustlines yet

@abresas abresas changed the title Xrpl multisig prover XRPL Multisig Prover Jan 3, 2024
@cjcobb23
Copy link
Contributor

cjcobb23 commented Jan 3, 2024

Why do we need a separate voting verifier for XRPL? We added support for succeeded vs failed on chain to the voting verifier, I think that's the only thing you needed for the XRPL case, is that right?

@abresas abresas force-pushed the xrpl-multisig-prover branch from d2bb4da to fa8111d Compare January 3, 2024 17:00
@k4m4
Copy link

k4m4 commented Jan 3, 2024

Why do we need a separate voting verifier for XRPL? We added support for succeeded vs failed on chain to the voting verifier, I think that's the only thing you needed for the XRPL case, is that right?

@cjcobb23 We had added status confirmation logic to the voting verifier before the feature you mentioned was implemented, so we had forked it out as a separate project. We recently discussed switching back to the voting verifier, but it looked like the feature we need is still WIP, since we couldn't find a way to query for the message status (Vote) that reached consensus (just boolean):

== Some(Vote::SucceededOnChain) // TODO: consider Vote::FailedOnChain?

@cjcobb23
Copy link
Contributor

cjcobb23 commented Jan 3, 2024

Why do we need a separate voting verifier for XRPL? We added support for succeeded vs failed on chain to the voting verifier, I think that's the only thing you needed for the XRPL case, is that right?

@cjcobb23 We had added status confirmation logic to the voting verifier before the feature you mentioned was implemented, so we had forked it out as a separate project. We recently discussed switching back to the voting verifier, but it looked like the feature we need is still WIP, since we couldn't find a way to query for the message status (Vote) that reached consensus (just boolean):

== Some(Vote::SucceededOnChain) // TODO: consider Vote::FailedOnChain?

I see, yes the status is not yet exposed through query. I think it would be preferable to just expose the status through a query in the voting verifier, rather than write an entirely new contract. We would welcome a PR for the voting verifier to expose the status via query.

@eguajardo
Copy link
Contributor

Why do we need a separate voting verifier for XRPL? We added support for succeeded vs failed on chain to the voting verifier, I think that's the only thing you needed for the XRPL case, is that right?

@cjcobb23 We had added status confirmation logic to the voting verifier before the feature you mentioned was implemented, so we had forked it out as a separate project. We recently discussed switching back to the voting verifier, but it looked like the feature we need is still WIP, since we couldn't find a way to query for the message status (Vote) that reached consensus (just boolean):

== Some(Vote::SucceededOnChain) // TODO: consider Vote::FailedOnChain?

I see, yes the status is not yet exposed through query. I think it would be preferable to just expose the status through a query in the voting verifier, rather than write an entirely new contract. We would welcome a PR for the voting verifier to expose the status via query.

I just started taking a look at this, I'll be working on the query to expose the status

message_id: &nonempty::String,
) -> Result<(nonempty::String, u64), ContractError> {
// expected format: <tx_id>:<index>
let components = message_id.split(":").collect::<Vec<_>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a standard format for message ids. This is used by evm chains, but other chains could use different formats. What types of messages is this called on specifically?

@k4m4 k4m4 force-pushed the xrpl-multisig-prover branch 2 times, most recently from 0341515 to b4da970 Compare April 18, 2024 16:39
@k4m4
Copy link

k4m4 commented Aug 23, 2024

Let's close this, since the XRPL-specific smart contracts will live in a Common Prefix fork. We will submit a separate PR with just the XRPL ampd components shortly.

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.

4 participants