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.

Sign up for GitHub

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

Jump to bottom

Replace gas price with fee, or remove it #3173

Merged
merged 17 commits into from
Nov 24, 2022
Merged

Replace gas price with fee, or remove it #3173

merged 17 commits into from
Nov 24, 2022

Conversation

gterzian
Copy link
Contributor

  • document all added functions
  • try in sandbox /simulation/labnet
  • unit tests on the added/changed features
    • make tests compile
    • make tests pass
  • add logs allowing easy debugging in case the changes caused problems
  • if the API has changed, update the API specification

@gterzian gterzian mentioned this pull request Oct 20, 2022
Closed
7 tasks
@gterzian gterzian force-pushed the replace_gas_price_with_fee_2 branch from f4e8e6c to 05b1d25 Compare October 21, 2022 08:46
@gterzian gterzian marked this pull request as ready for review October 21, 2022 09:21
@gterzian gterzian requested a review from damip October 21, 2022 09:28
@gterzian
Copy link
Contributor Author

@damip Thanks for the review, it is ready for another round. Still wondering about #3173 (comment)

@gterzian gterzian changed the base branch from main to testnet_17 October 24, 2022 07:59
@gterzian gterzian force-pushed the replace_gas_price_with_fee_2 branch 2 times, most recently from 2322116 to 3ad0d60 Compare November 2, 2022 09:07
@gterzian
Copy link
Contributor Author

gterzian commented Nov 2, 2022 •
edited
Loading

Tested this with the simulator(using the branch of massalabs/massa-sc-runtime#156 for the runtime). The charge.py script ran without problems. cc @damip @aoudiamoncef

@gterzian gterzian force-pushed the replace_gas_price_with_fee_2 branch from 3ad0d60 to 4f97e45 Compare November 3, 2022 08:52
@gterzian
Copy link
Contributor Author

gterzian commented Nov 3, 2022

@aoudiamoncef Could you please take a look at the changes to openrpc.json in the light of massalabs/massa-sc-runtime#156 (comment)?

@gterzian gterzian mentioned this pull request Nov 4, 2022
Merged
@gterzian
Copy link
Contributor Author

gterzian commented Nov 7, 2022

@damip @aoudiamoncef Should we try to merge both this one and massalabs/massa-sc-runtime#156 ?

@damip
Copy link
Member

damip commented Nov 7, 2022

@gterzian there are some unadressed comments in this PR. Can you address them all ?

@gterzian
Copy link
Contributor Author

gterzian commented Nov 8, 2022

there are some unadressed comments in this PR. Can you address them all ?

Yes, thanks for pointing those out... @damip

@gterzian gterzian marked this pull request as draft November 8, 2022 08:29
@gterzian
Copy link
Contributor Author

gterzian commented Nov 8, 2022 •
edited
Loading

@damip Note that the AsyncMessageId changes change the behavior of the take_batch_to_execute method of AsyncPool, see this change.

Is this intended?

@gterzian gterzian mentioned this pull request Nov 9, 2022
Merged
@gterzian
Copy link
Contributor Author

gterzian commented Nov 9, 2022

Ok I think we can review as a package this PR, as well as:

@damip @aoudiamoncef

@gterzian gterzian marked this pull request as ready for review November 9, 2022 09:09
@gterzian
Copy link
Contributor Author

Ok the runtime dependency now points to the testnet_17 branch of the runtime

@AurelienFT AurelienFT mentioned this pull request Nov 23, 2022
Closed
@gterzian
Copy link
Contributor Author

If you tested it in local with the others repository. Ok to merge it.

@AurelienFT ran charge.py with the simulator on it without problems...

@AurelienFT
Copy link
Contributor

@AurelienFT ran charge.py with the simulator on it without problems...

I think you can fix conflicts and merge it so.

@gterzian gterzian force-pushed the replace_gas_price_with_fee_2 branch from 7fb0e85 to cfcc328 Compare November 24, 2022 09:18
@gterzian
Copy link
Contributor Author

gterzian commented Nov 24, 2022 •
edited
Loading

tests::scenarios::test_bootstrap_server starting to fail again after the rebase...

Actually it's intermittent, I've opened #3241

Should we merge this one? @AurelienFT

@gterzian gterzian mentioned this pull request Nov 24, 2022
Closed
@Eitu33
Copy link
Contributor

Eitu33 commented Nov 24, 2022

Let's merge this imho, I'll take care of the bootstrap test #3241 (comment)

@AurelienFT AurelienFT merged commit 1cfe1ea into testnet_17 Nov 24, 2022
bors bot added a commit that referenced this pull request Nov 30, 2022
@bors @AurelienFT
Merge #3181
5044f28 
3181: Testnet 17 r=AurelienFT a=gterzian

This PR is a batch for testnet 17. These PRs should be merged before merging this one : 

- #3202 
- #3205
- #3162
- #3203
- #3232
- #3192
- #3229 
- #3221 
- #3219 
- #3210 
- #3191 
- #3173 
- massalabs/massa-sc-runtime#161
- massalabs/massa-sc-runtime#164
- #3253 
- #3249 
- #3238 

After merging this PR we should merge these before release testnet 17 : 
- massalabs/massa-as-sdk#49
- massalabs/massa-as-sdk#39

Co-authored-by: AurelienFT <[email protected]>
@AurelienFT AurelienFT mentioned this pull request Dec 1, 2022
Closed
@AurelienFT AurelienFT deleted the replace_gas_price_with_fee_2 branch May 29, 2023 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damip damip damip left review comments

@Eitu33 Eitu33 Eitu33 approved these changes

@AurelienFT AurelienFT AurelienFT approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gterzian @damip @AurelienFT @Eitu33