amendment: assign the serial number to the amendment list object #4602
Conversation
Tracking the serial number allows for check if amendments have changed since last check without checking of the list one by one
| @@ -329,6 +329,15 @@ Change::applyAmendment() | |||
| << " activated: server blocked."; | |||
| ctx_.app.getOPs().setAmendmentBlocked(); | |||
| } | |||
|
|
|||
| if (view().rules().enabled(featureAmendmentTableSequence) || | |||
There was a problem hiding this comment.
It might be nice to increment the serial number when amendments gain or loose majority too since those change the amendment ledger entry.
There was a problem hiding this comment.
That makes the serial nomber not very useful to check if amendments have made a changed which is a big use case for the rules of transaction process.
|
Do maintaineurs check+merge changes requests from people not working in Ripple company? I try to make changes but nothing happens. But the Ripple changes will merge very rapidly. Please waiting for your response @JoelKatz, @manojsdoshi, @N3TC4T, @nbougalis, @nixer89, @RichardAH, @seelabs, @Silkjaer, @WietseWind, @ximinez. |
|
Time and resources are limited, but in general, yes - RippleX engineers make an effort to review contributions from external developers in the community. We have other priorities, so it may take some time for us to get to your PR, but we will. Can you share the motivations for this PR? How is this change beneficial? How do you weigh that against the downsides (more code/complexity, more memory/compute use, etc)? |
|
The serial number allows for easy check of if new amendments have activated and with it server does not need to load and process the amendment object that has not been changed. It has very low complexité and no downsides for computer memory or more complex. |
At least for now, once an amendment is enabled - that is, added to the Footnotes
|
| @@ -141,6 +141,7 @@ LedgerFormats::LedgerFormats() | |||
| { | |||
| {sfAmendments, soeOPTIONAL}, // Enabled | |||
| {sfMajorities, soeOPTIONAL}, | |||
| {sfSequence, soeOPTIONAL}, | |||
There was a problem hiding this comment.
Why have a separate Sequence field instead of just using the existing logic for sfPreviousTxnID/sfPreviousTxnLgrSeq?
There was a problem hiding this comment.
I would prefer those fields too. It would allow walking the activation change txs.
There was a problem hiding this comment.
#4751 - IMO this is a better solution since it aligns with the existing ledger structure
|
Hi @a-noni-mousse - do you have any thoughts on the comments above? It should be straightforward to compare the amendment object itself to see if there have been changes; and if the format of the data is a problem, you could compare a hash of the object in order to detect changes. |
|
If there's no reply for a while, this PR will be closed due to inactivity. You'll always be welcome to open a new PR (or reopen this one) if you still feel the change is worth making. |
|
Closing due to inactivity. Suggested alternatives are detailed above, e.g. compare the amendment object itself, or a hash of that object. However, if that is insufficient for your use case, please feel free to re-open, or open a new PR. |
Tracking the serial number allows for check if amendments have changed since last check without checking of the list one by one
High Level Overview of Change
Context of Change
Type of Change
Before / After
When amendment activates the new field Sequence in the amendments object will be increased 1. Before the field does not exist.