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

fix: Add v53 to config store #6556

Merged
merged 4 commits into from
Apr 8, 2022
Merged

Conversation

jakmeier
Copy link
Contributor

@jakmeier jakmeier commented Apr 8, 2022

When updating #6397 to protocol v53 right before merging,
I forgot to add the configuration JSON file to the config store.

Test plan

Added a test to verify that cost indeed increases.

When updating near#6397 to protocol v53 right before merging,
I forgot to add the configuration JSON file to the config store.

Test plan
---------
Added a test to verify that cost indeed increases.
@jakmeier jakmeier marked this pull request as ready for review April 8, 2022 01:00
@jakmeier jakmeier requested a review from a team as a code owner April 8, 2022 01:00
Comment on lines 3501 to 3509
let tip = env.clients[0].chain.head().unwrap();
let signed_transaction =
Transaction { nonce: 10, block_hash: tip.last_block_hash, ..tx.clone() }.sign(&signer);
let tx_hash = signed_transaction.get_hash();
env.clients[0].process_tx(signed_transaction, false, false);
for i in 0..epoch_length {
env.produce_block(0, tip.height + i + 1);
}
env.clients[0].chain.get_final_transaction_result(&tx_hash).unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps wrap it in a closure so we can reuse it rather than duplicating the same code? This will make it obvious that we deploy the exact same contract.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I like that. Done.

Comment on lines 3465 to 3466
let old_protocol_version = 52;
let new_protocol_version = 53;
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively I would introduce a protocol feature like IncreaseDeploymentCost and use its protocol version (- 1).

I think of a scenario when we can postpone this feature to the v54 due to emergency reasons. Then it will be natural to modify protocol version of feature in the version.rs, but it can be easy to forget to fix two numbers in some test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I have now added this as an explicit protocol feature.

1) Add ProtocolFeature::IncreaseDeploymentCost
2) Move duplicated code into closure
@jakmeier jakmeier requested a review from Longarithm April 8, 2022 11:20
@Longarithm
Copy link
Member

Thank you!

@jakmeier jakmeier merged commit c81df9a into near:master Apr 8, 2022
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