xrpl.js icon indicating copy to clipboard operation
xrpl.js copied to clipboard

add ExpandedSignerList amendment support

Open khancode opened this issue 2 years ago • 5 comments

High Level Overview of Change

Adds ExpandedSignerList amendment support.

Context of Change

  • Expands the maximum signer list size to 32 entries.
  • Adds an optional field WalletLocator to SignerEntry.
  • rippled PR

Type of Change

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] Refactor (non-breaking change that only restructures code)
  • [ ] Tests (You added tests for code that already exists, or your new feature included in this PR)
  • [ ] Documentation Updates
  • [ ] Release

Test Plan

Adds unit & integration tests.

Future Tasks

Merge this PR once the amendment goes live.

khancode avatar Jun 14 '22 21:06 khancode

It is possible to rename the field to SignerMetadata since the actual data is in a binary format, and clients can interpret (map) / encode any field names they want. But given the code in rippled and on the docs, keeping the WalletLocator name is certainly the path of least resistance

intelliot avatar Jun 17 '22 00:06 intelliot

It is possible to rename the field to SignerMetadata since the actual data is in a binary format, and clients can interpret (map) / encode any field names they want. But given the code in rippled and on the docs, keeping the WalletLocator name is certainly the path of least resistance

@intelliot I believe the better path is to use the same parameter name, WalletLocator, that's on the docs to avoid confusion. AFAIK, we've been consistent with our models containing the same field names as what's on the docs. If users do get confused by the name, they can read the JSDocs or the official docs to understand its usage. In addition, we can propose the name change SignerMetadata to rippled team and see if they can refactor the name and publish the change. Not sure if this would have to be in a different amendment or if they can modify the amendment?

khancode avatar Jun 17 '22 15:06 khancode

@intelliot I believe the better path is to use the same parameter name, WalletLocator, that's on the docs to avoid confusion. AFAIK, we've been consistent with our models containing the same field names as what's on the docs. If users do get confused by the name, they can read the JSDocs or the official docs to understand its usage.

Re: the first part, just wanted to add +1 to staying consistent with rippled's naming/interface.

JST5000 avatar Jun 17 '22 16:06 JST5000

The rippled team is aware of this and there's no need to change it. It was discussed in the PR you linked to, https://github.com/ripple/rippled/pull/4097

There's nothing strictly canonical about the JSON format: only the binary really matters at a protocol level. But given that rippled is currently the only implementation, I agree that we should just use the same names unless there is a very good reason not to.

intelliot avatar Jun 17 '22 17:06 intelliot

  1. Can we add a caveat in the release notes to say that the limit is 8 unless the ExpandedSignerList amendment is active?
  2. What error should users expect if they attempt to set 9+ signers on a network where ExpandedSignerList is not active? It may be useful to document this in the release notes (or elsewhere) as well.

intelliot avatar Jul 19 '22 13:07 intelliot