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

add ExpandedSignerList amendment support #2026

Merged
merged 12 commits into from
Sep 13, 2022
Merged

Conversation

khancode
Copy link
Collaborator

@khancode khancode commented Jun 14, 2022

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)
  • 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 khancode requested review from ckniffen and JST5000 June 14, 2022 21:57
Copy link
Contributor

@JST5000 JST5000 left a comment

Choose a reason for hiding this comment

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

LGTM

@intelliot
Copy link
Collaborator

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

@khancode
Copy link
Collaborator Author

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?

@JST5000
Copy link
Contributor

JST5000 commented Jun 17, 2022

@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.

@intelliot
Copy link
Collaborator

The rippled team is aware of this and there's no need to change it. It was discussed in the PR you linked to, XRPLF/rippled#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
Copy link
Collaborator

  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.

@khancode khancode changed the base branch from main to develop September 13, 2022 17:40
@khancode khancode merged commit 8a9a9bc into develop Sep 13, 2022
@khancode khancode deleted the expanded-signer-list branch September 13, 2022 17:50
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