rippled icon indicating copy to clipboard operation
rippled copied to clipboard

Published 20 hours ago • verified publisher icon XRPLF

Simplify processing and verifying signer lists:

Open a-noni-mousse opened this issue 4 years ago • 4 comments

I am trying this PR again.

The commit reduces the scope of the signer list parsing code to the Transactor and simplifies the SetSignerList transactor. It introduces an amendment because it requries the signer list to be ordered when submitting transactions that modify the list.

High Level Overview of Change

Context of Change

Type of Change

  • [X] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [X] Tests (You added tests for code that already exists, or your new feature included in this PR)

a-noni-mousse avatar Jun 06 '21 10:06 a-noni-mousse

Thanks for resubmitting the PR. I'll try to get to this later this week.

Edit: Sorry, I don't have time to get to this right now. But rather than let it wait another developer offered to review it in my place.

seelabs avatar Jun 08 '21 12:06 seelabs

I don't think it makes sense to require the signer list to be ordered when submitting transactions that modify the list.

As @thejohnfreeman suggested, each new amendment carries some cost. Additionally, this change could cause some previously-valid transactions to become invalid. So it also affects external tooling that manages Signers, likely requiring updates to other projects outside of rippled.

Next steps:

  • If there is a benefit to simplifying the code in this way, what exactly is that benefit?
  • Consider modifying the proposed change such that no amendment is required, and no previously-valid transactions would become invalid.

We welcome your contributions. Thanks again for opening this PR.

intelliot avatar Jun 10 '21 19:06 intelliot

I think that the requirement of sorting the list helps ensure a canonical repreesntation of the transaction but it would require the change in all librries so yes you are right not a good idea to do it. I can change that but many of my changes get stuck and nothing happens with them which is annoying thing.

a-noni-mousse avatar Jul 29 '22 19:07 a-noni-mousse

I had reviewed this 4 days after it was submitted, and none of the questions were addressed in over a year. Why is it stuck?

thejohnfreeman avatar Aug 04 '22 14:08 thejohnfreeman

(notes: not ready to merge; waiting for response from author)

If we don't hear from @a-noni-mousse in 1 month, we'll close this.

intelliot avatar Nov 28 '22 19:11 intelliot

Closing due to inactivity. @a-noni-mousse if you'd like to return to this PR, feel free to re-open or open a new PR.

intelliot avatar Feb 17 '23 06:02 intelliot