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 new phases in getUpgradePhases and NetworkId constants #97

Merged
merged 11 commits into from
Aug 7, 2024

Conversation

VjeraTurk
Copy link

@VjeraTurk VjeraTurk commented Jul 29, 2024

Why this should be merged

For feature flag to work with Berlin Phase (and defined future or past phases in the same way )

How this works

getUpgradePhases propagates a call to the node and answers whether the phase is activated.
Answers NaN if the phase is not defined in caminogo node.

How this was tested

By running the Example for getUpgradePhases (added Webstorm Run file)

NOTE POST MERGE:
Must be published the old way - using dist branch the way @mo-c4t did for rc-1

@VjeraTurk VjeraTurk marked this pull request as draft July 29, 2024 11:27
@VjeraTurk VjeraTurk requested a review from Ysrbolles July 29, 2024 11:28
Copy link

github-actions bot commented Jul 29, 2024

⚠️ Found errors/fatal log records. Please review them(job:e2e, step:"Check produced logs") and resolve this comment.

2 errors
0 fatal

@VjeraTurk VjeraTurk requested a review from evlekht July 29, 2024 12:41
src/utils/constants.ts Show resolved Hide resolved
@@ -358,7 +358,8 @@ export interface ClaimAmountParams {
}

export interface UpgradePhasesReply {
SunrisePhase: number
Copy link
Member

Choose a reason for hiding this comment

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

why remove sunrise phase?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

its active camino phase

Copy link
Author

Choose a reason for hiding this comment

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

It's back, but made optional, with all the other phases

@VjeraTurk VjeraTurk self-assigned this Jul 30, 2024
@VjeraTurk VjeraTurk marked this pull request as ready for review July 30, 2024 15:24
@VjeraTurk VjeraTurk requested a review from evlekht July 30, 2024 16:08
export const ColumbusID = "1001"
export const CaminoID = "1000"

export const BERLINPHASETIME: Map<string, Date> = new Map([
Copy link
Member

Choose a reason for hiding this comment

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

please add athens and sunrise as well, so we'll have it in js sdk

Copy link
Member

Choose a reason for hiding this comment

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

also, I would suggest to add some default timestamp for unknown network ids

@@ -0,0 +1,5 @@
<component name="ProjectRunConfigurationManager">
<configuration default="false" name="Get Upgrade Phases" type="NodeJSConfigurationType" application-parameters="examples/platformvm/getUpgradePhases.ts" path-to-node="$USER_HOME$/.nvm/versions/node/v16.20.2/bin/node" node-parameters="-r tsconfig-paths/register" path-to-js-file="node_modules/ts-node/dist/bin.js" working-dir="$PROJECT_DIR$">
Copy link
Member

Choose a reason for hiding this comment

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

what's this file used for ? are we still using node 16 in caminojs ?

Copy link
Author

@VjeraTurk VjeraTurk Aug 2, 2024

Choose a reason for hiding this comment

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

This is a Webstorm version of run file such as "launch.json" is in VS code.

It helps to run the examples within the WebStorm IDE and thus enables debugging within the IDE.

Node is down to v16.20.0 cause otherwise the examples don't run - I can adjust the examples to be compatible with higher versions. It's only a matter of awaiting of the main function within the examples

Copy link
Member

Choose a reason for hiding this comment

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

if its ide specific, it should be gitignored

Copy link
Author

Choose a reason for hiding this comment

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

Gitignored. In the next release, I will increase the node version required to run the examples and add to README.md how to set up run files (to enable examples debugging) via IDE-specific run files.

export const BERLINPHASETIME: Map<string, Date> = new Map([
[KopernikusID, new Date("2024-04-13T00:00:00Z")],
[ColumbusID, new Date("2024-08-07T00:00:00Z")],
[CaminoID, new Date("2024-08-17T00:00:00Z")]
Copy link
Member

Choose a reason for hiding this comment

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

for Berlin, I think this date is too early.

Copy link
Author

Choose a reason for hiding this comment

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

@mo-c4t Do you know where it is possible to check the dates? The only source I have is https://github.com/chain4travel/caminogo/blob/dev/version/constants.go#L184

Copy link
Member

Choose a reason for hiding this comment

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

We still don't have final dates. but I think you can put the dates more into the future from Columbus and Camino until we have final dates. what do you think @evlekht ?

Copy link
Member

Choose a reason for hiding this comment

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

yes, ofc, put something temporal

@VjeraTurk
Copy link
Author

@SamJaarsma we need the dates for Berlin and Cairo if possible

@evlekht
Copy link
Member

evlekht commented Aug 5, 2024

After discussion on currently daily we decided to use pvm getUpgradePhases api for now, because we'll be able to hardcode only camino and columbus network phase timestamps. For the rest we need node api that will return actual timestamps, but we don't have such api yet.

@VjeraTurk VjeraTurk merged commit fbc7ea9 into dev Aug 7, 2024
6 checks passed
@VjeraTurk VjeraTurk changed the title Add phase timestamps as Caminojs constant for DAC #607 Add new phases in getUpgradePhases and NetworkId constants Aug 7, 2024
@VjeraTurk VjeraTurk deleted the VjeraTurk/feature-flag-for-dac-timestamp branch November 15, 2024 14:39
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.

3 participants