-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: add amendments enabled on each network #133
Conversation
…ator-history-service into add-amendment-info
…alidator-history-service into add-amendments-enabled
| `networks` |The network where the amendment has been enabled. | | ||
| `ledger_index` |The ledger where the amendment has been enabled. | | ||
| `tx_hash` |The transaction hash where the amendment has been enabled.| | ||
| `date` |The date and time when the amendment has been enabled. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this in ripple epoch time or POSIX time? (Please include in the description here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's converted to js Date
type instead of a number so I'm not sure it needs clarification here (it will show YYYY-MM-DD HH:MM:SS in the database).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's still important to mention because that changes how you interpret the number. (The Date
type is how it's formatted, but knowing if it's ripple epoch changes what that date represents. If you directly change a ripple epoch number into a Date, it'll give you the wrong date)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd probably be easier to store it in ripple epoch (or UNIX epoch), tbh. You can convert when outputting.
Co-authored-by: Jackson Mills <[email protected]>
Co-authored-by: Jackson Mills <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
// in a network that doesn't vote for a new fee. We want to do it only once | ||
// per network. This part will check if the node is the initial node that we |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want to do this only once per network?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And why not make that a separate call that happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion, my comment for why this is being done here was incorrect (it had been a while until I added that comment to explain to Jackson). This should be for using the ledger_entry method to get the list of enabled amendments at the time when connections start or a new network being added (to capture amendments enabled on genesis). This is done only once per network, and then later on new enabled amendments will be added via searching for EnableAmendment tx, which would provide more details compared to ledger_entry. I have updated the comment to correctly explain what's going on here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other details for these already added amendments will also be added manually via the amendments_enabled.json
file (but only ones that had been enabled via EnableAmendment tx in the past, those are added on genesis will have ledger_index
, tx_hash
and date
fields blank
networks: string | ||
ledger_index: number | ||
tx_hash: string | ||
date: number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The date should be different on every network. This data model makes it seem like the date is shared.
const filePath = 'src/shared/data/amendments_enabled.json' | ||
|
||
interface AmendmentEnabledJson { | ||
networks: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not be called networks because each are different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is called networks
across other tables as well even though all should be a 1-1 match. I have created an issue to fix them:
#165
High Level Overview of Change
Create a table with the following information to track amendments enabled on each network:
AmendmentID
NetworkID
ledger_index
tx_hash
date
Also add a function to manually update the table using a JSON file (runs whenever re-deployed).
Type of Change
Test Plan
Works on staging.