feat: add comprehensive contribution guidelines #117
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I'm pretty sure I'm doing things wrong here, but it's my best attempt so far. This is the gist of the changes: - Use only one package.json (I used hardhat-ethers as a reference implementation) - Set up hardhat.config.ts on the toplevel - Publish niljs build artifacts that contain a dist/ directory (will be needed when building in nix) Tests still don't run yet, but I feel I'm getting close enough.
This essentially fixes a problem with dependencies for our JS modules. There is still separation of concerns for testing phase and (somewhat) for the build phase. But at least now we can have straightforward builds with Nix.
For niljs this value is hardcoded. But it's a temporary solution to make tests work.
switch of tests to new globals and npm concurrent tests + config update set docs test to specific rev added endpoint switcher and script Update contract code and calldata in load_generator Move Account initialization to account_state.go Signed-off-by: Mikhail Sherstennikov <[email protected]> Improve error handling in precompiles - Add new status `MessageStatusPrecompileReverted`, which indicate that a common error was occurred during precompile execution. - Force all precompiles to return correct error type. - Added several tests for precompile failures that can be initiated trivially. - No need to return bool in `Nil.asyncCall`, because if `asyncCall` failed, it returns boolean flag implicitly. TODO: Some precompiles return failure message in return data, but for now there are not propagated to final return data. Thus, it is not visible for ExecutionState. Closes #629 Signed-off-by: Mikhail Sherstennikov <[email protected]> Enable hardhat plugin testing during CI This diff's purpose is to enable running tests for hardhat in CI, and also serve as an example of how a test launcher can be written in bash. At this moment, hardhat tests need you to manually start `nild`, as they don't do it themselves. Fortunately, it's quite easy to do so in a wrapper shell script, which I've implemented. The shell script will make sure that `nild` is killed if the tests fail by setting a `trap` for common signals (such as SIGINT or EXIT), so that you don't have to worry about hanging background processes. This is quite standard for writing automation shell scripts. removed revision error (#763) Update contract code and calldata in load_generator Move Account initialization to account_state.go Signed-off-by: Mikhail Sherstennikov <[email protected]> Improve error handling in precompiles - Add new status `MessageStatusPrecompileReverted`, which indicate that a common error was occurred during precompile execution. - Force all precompiles to return correct error type. - Added several tests for precompile failures that can be initiated trivially. - No need to return bool in `Nil.asyncCall`, because if `asyncCall` failed, it returns boolean flag implicitly. TODO: Some precompiles return failure message in return data, but for now there are not propagated to final return data. Thus, it is not visible for ExecutionState. Closes #629 Signed-off-by: Mikhail Sherstennikov <[email protected]> Enable hardhat plugin testing during CI This diff's purpose is to enable running tests for hardhat in CI, and also serve as an example of how a test launcher can be written in bash. At this moment, hardhat tests need you to manually start `nild`, as they don't do it themselves. Fortunately, it's quite easy to do so in a wrapper shell script, which I've implemented. The shell script will make sure that `nild` is killed if the tests fail by setting a `trap` for common signals (such as SIGINT or EXIT), so that you don't have to worry about hanging background processes. This is quite standard for writing automation shell scripts. removed revision error (#763) Fix install_soljson.sh for macosx remove migration workaround (#767) load_generator: drop hardcode After this patch we will take test contract code from VFS instead of hardcoded string. Docs new integration tests (#769) Signed-off-by: Mikhail Sherstennikov <[email protected]> Co-authored-by: Zerg1996 <[email protected]> Co-authored-by: Mikhail Sherstennikov <[email protected]> Co-authored-by: Konstantin Nazarov <[email protected]> Separate build and check phases for nil.js and docs addressed suggestions about hardhat and seqno (#774) add "feeCredit" for external message To replace hardcoded value in a code. add externalFeeCredit for clients For niljs this value is hardcoded. But it's a temporary solution to make tests work. Add abi source code We need it because we have types that are not supported by original abi implementation, such as `types.Uint256`. Signed-off-by: Mikhail Sherstennikov <[email protected]> Support `types.Uint256` in abi packing Signed-off-by: Mikhail Sherstennikov <[email protected]> Docs tutorials tests: CLI (#779) Signed-off-by: Mikhail Sherstennikov <[email protected]> Co-authored-by: Zerg1996 <[email protected]> Co-authored-by: Mikhail Sherstennikov <[email protected]> Co-authored-by: Konstantin Nazarov <[email protected]> Co-authored-by: Dmitrii Skopintsev <[email protected]> Co-authored-by: Miron <[email protected]> Co-authored-by: Oleg Babin <[email protected]> update golangci-lint version to 1.60.2 Support on-chain config parameters Config trie is stored in the main shard. Regular shards read config via reference to the block in main shard. Only contracts from main shard can modify config params. Currently, there are two config parameters: validators list and gas price scale. Config params can be accessed from Solidity contracts. Signed-off-by: Mikhail Sherstennikov <[email protected]> Replace concurrent.OnSignal with signal.NotifyContext shutdown eth client routines before closing db bump golangci-lint to 1.60.3 reduce code duplication between rpc client and direct client Fix docs test running scripts (#782) Signed-off-by: Mikhail Sherstennikov <[email protected]> Co-authored-by: khannanov-nil <[email protected]> Co-authored-by: Zerg1996 <[email protected]> Co-authored-by: Mikhail Sherstennikov <[email protected]> Co-authored-by: Konstantin Nazarov <[email protected]> Co-authored-by: khannanov-nil <[email protected]> Co-authored-by: Miron <[email protected]> Co-authored-by: Oleg Babin <[email protected]> Yaml config for nild Retry requests to faucet with increasing seqno use "pending" when query seqno instead of "latest" In case of two concurrent requests we will have an error because "latest" checks committed seqno. "Pending" checks also message pool. Fix `StaticCall` opcode Closes #786 Signed-off-by: Mikhail Sherstennikov <[email protected]> Fix flag ordering in nild feat(utils): add RunTickerLoop function for managing ticker-based loops Add a new utility function `RunTickerLoop` to encapsulate common ticker-based loop patterns found throughout the codebase. This function provides a reusable way to execute tasks at regular intervals while respecting context cancellation. feat: add sync committee node (dummy) Add another binary `sync_committee` containing sync committee node. For now it is dummy — fetches latest block from each shard, if number is higher than previous fetched block for this shard, the range of blocks is fetched. Then it calls dummy `createProofTasks`, where only main shard blocks are used (as expected in testnet), others are removed from the storage. This is a preliminary implementation and does not yet create actual proof tasks. Some basic metrics and telemetry included. Usage example: ```bash ./build/bin/sync_committee run --log-level debug --delay 5s ./build/bin/sync_committee --help ``` Run doc tests in flake check (#797) Signed-off-by: Mikhail Sherstennikov <[email protected]> Co-authored-by: khannanov-nil <[email protected]> Co-authored-by: Zerg1996 <[email protected]> Co-authored-by: Mikhail Sherstennikov <[email protected]> Co-authored-by: Konstantin Nazarov <[email protected]> Co-authored-by: khannanov-nil <[email protected]> Co-authored-by: Miron <[email protected]> Co-authored-by: Oleg Babin <[email protected]> implement eth_estimateFee API This patch adds the eth_estimateFee API to estimate funds needed for sending a message. This call is not identical to the original eth_estimateGas because gas prices may vary across shards. Therefore, in our case, we get an estimate of the funds required for the near future (and this may easily change over time). The algorithm is fairly simple, though it needs explanation. We cannot simply execute eth_call with an overridden contract balance because the gas consumed may be less than what is required during the call (see EIP2200). So, we override the account balance and use binary search to vary feeCredit. A separate issue here is that this process must be done for all messages sent. That is, if we have an external message that leads to an internal message, we must first estimate the fee for the internal message and then for the external message. We do not unpack messages and try not to touch user data. This can be done later. In tests, hardcoded constants were replaced with empty/null values. If the user does not explicitly specify a value, fee estimation is triggered. This allows for more comprehensive code testing. However, it is not possible to completely eliminate this, as we have negative tests that check for errors. It is expected that this will happen asynchronously, while fee estimation is a fully synchronous operation. Enable Github CI for the merge queue Apparently, it's not enough to just enable the merge queue. We also need to make a change to the Actions Workflow, as per https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue#triggering-merge-group-checks-with-github-actions Without this change, merge queue entries will hang indefinitely. Add `IncludedInMain` flag to `RPCReceipt` This flag indicates whether receipt's block is included in the main chain. Relates #800 Signed-off-by: Mikhail Sherstennikov <[email protected]> add tests for minter in hardhat send tokens to another contract fetch endpoint from .env fix tests and improve precompiles copy precompiles everywhere fix tests implement hardhat test for awaitCall add editorconfig to the root for js packages Standard error for missing config keys in nil cli additional Nil.js tests in docs fix to tests Don't needlessly poison build caches. Set version by binary patch I've noticed that every time you commit something locally, the next time you run "nix build", the `nil` derivation always gets rebuilt. This was in particular the case during changing `docs` or `nil.js`, which in theory shouldn't affect `nil` itself. Then I've realized that we pass current revision and commit number as an input to the `nil` derivation, and we embed those into the binary at the build phase. All this means that even though the binary is functionally exactly the same, we will needlessly re-run tests and essentially rebuild everything every time even if we just change something in `hardhat-plugin`. This diff is implementing a simple and straightforward trick of injecting the version number into the binary at the packing phase. At the build phase we just add a pre-defined 40-byte string that can be easily replaced by `sed` (see the `scripts/binary_patch_version.sh` script). By doing so, I'm saving people countless number of needless rebuilds. This should also positively affect the CI as well. Fix timeouts in filters tests Signed-off-by: Mikhail Sherstennikov <[email protected]> Update NotReplaced error to more explicit condition test: fix flaky tests Sometimes (especially locally) some getters tests failed because we didn't have result of deployment/execution committed into main block. add nix/sh formatters Ported from `dbms`. Improve nild config: restructure, rename, add db Remove duplication of npmDeps in JS subprojects Currently there are a few places where you have to update the hash of npmDeps every time it changes. I've moved the fetching of the npm package dependencies to `npmdeps.nix` which removes this duplication and makes things easier to reason about. bump libp2p version Add call + support ABI + args for syncCall/sendCall on WalletV1 Update contract code and calldata in load_generator Move Account initialization to account_state.go Signed-off-by: Mikhail Sherstennikov <[email protected]> Improve error handling in precompiles - Add new status `MessageStatusPrecompileReverted`, which indicate that a common error was occurred during precompile execution. - Force all precompiles to return correct error type. - Added several tests for precompile failures that can be initiated trivially. - No need to return bool in `Nil.asyncCall`, because if `asyncCall` failed, it returns boolean flag implicitly. TODO: Some precompiles return failure message in return data, but for now there are not propagated to final return data. Thus, it is not visible for ExecutionState. Closes #629 Signed-off-by: Mikhail Sherstennikov <[email protected]> Enable hardhat plugin testing during CI This diff's purpose is to enable running tests for hardhat in CI, and also serve as an example of how a test launcher can be written in bash. At this moment, hardhat tests need you to manually start `nild`, as they don't do it themselves. Fortunately, it's quite easy to do so in a wrapper shell script, which I've implemented. The shell script will make sure that `nild` is killed if the tests fail by setting a `trap` for common signals (such as SIGINT or EXIT), so that you don't have to worry about hanging background processes. This is quite standard for writing automation shell scripts. removed revision error (#763) Fix install_soljson.sh for macosx remove migration workaround (#767) load_generator: drop hardcode After this patch we will take test contract code from VFS instead of hardcoded string. Docs new integration tests (#769) Signed-off-by: Mikhail Sherstennikov <[email protected]> Co-authored-by: Zerg1996 <[email protected]> Co-authored-by: Mikhail Sherstennikov <[email protected]> Co-authored-by: Konstantin Nazarov <[email protected]> Separate build and check phases for nil.js and docs addressed suggestions about hardhat and seqno (#774) add "feeCredit" for external message To replace hardcoded value in a code. add externalFeeCredit for clients For niljs this value is hardcoded. But it's a temporary solution to make tests work. Add abi source code We need it because we have types that are not supported by original abi implementation, such as `types.Uint256`. Signed-off-by: Mikhail Sherstennikov <[email protected]> Support `types.Uint256` in abi packing Signed-off-by: Mikhail Sherstennikov <[email protected]> Docs tutorials tests: CLI (#779) Signed-off-by: Mikhail Sherstennikov <[email protected]> Co-authored-by: Zerg1996 <[email protected]> Co-authored-by: Mikhail Sherstennikov <[email protected]> Co-authored-by: Konstantin Nazarov <[email protected]> Co-authored-by: Dmitrii Skopintsev <[email protected]> Co-authored-by: Miron <[email protected]> Co-authored-by: Oleg Babin <[email protected]> update golangci-lint version to 1.60.2 Support on-chain config parameters Config trie is stored in the main shard. Regular shards read config via reference to the block in main shard. Only contracts from main shard can modify config params. Currently, there are two config parameters: validators list and gas price scale. Config params can be accessed from Solidity contracts. Signed-off-by: Mikhail Sherstennikov <[email protected]> Replace concurrent.OnSignal with signal.NotifyContext shutdown eth client routines before closing db bump golangci-lint to 1.60.3 reduce code duplication between rpc client and direct client Fix docs test running scripts (#782) Signed-off-by: Mikhail Sherstennikov <[email protected]> Co-authored-by: khannanov-nil <[email protected]> Co-authored-by: Zerg1996 <[email protected]> Co-authored-by: Mikhail Sherstennikov <[email protected]> Co-authored-by: Konstantin Nazarov <[email protected]> Co-authored-by: khannanov-nil <[email protected]> Co-authored-by: Miron <[email protected]> Co-authored-by: Oleg Babin <[email protected]> Yaml config for nild Retry requests to faucet with increasing seqno use "pending" when query seqno instead of "latest" In case of two concurrent requests we will have an error because "latest" checks committed seqno. "Pending" checks also message pool. Fix `StaticCall` opcode Closes #786 Signed-off-by: Mikhail Sherstennikov <[email protected]> Fix flag ordering in nild feat(utils): add RunTickerLoop function for managing ticker-based loops Add a new utility function `RunTickerLoop` to encapsulate common ticker-based loop patterns found throughout the codebase. This function provides a reusable way to execute tasks at regular intervals while respecting context cancellation. feat: add sync committee node (dummy) Add another binary `sync_committee` containing sync committee node. For now it is dummy — fetches latest block from each shard, if number is higher than previous fetched block for this shard, the range of blocks is fetched. Then it calls dummy `createProofTasks`, where only main shard blocks are used (as expected in testnet), others are removed from the storage. This is a preliminary implementation and does not yet create actual proof tasks. Some basic metrics and telemetry included. Usage example: ```bash ./build/bin/sync_committee run --log-level debug --delay 5s ./build/bin/sync_committee --help ``` Run doc tests in flake check (#797) Signed-off-by: Mikhail Sherstennikov <[email protected]> Co-authored-by: khannanov-nil <[email protected]> Co-authored-by: Zerg1996 <[email protected]> Co-authored-by: Mikhail Sherstennikov <[email protected]> Co-authored-by: Konstantin Nazarov <[email protected]> Co-authored-by: khannanov-nil <[email protected]> Co-authored-by: Miron <[email protected]> Co-authored-by: Oleg Babin <[email protected]> implement eth_estimateFee API This patch adds the eth_estimateFee API to estimate funds needed for sending a message. This call is not identical to the original eth_estimateGas because gas prices may vary across shards. Therefore, in our case, we get an estimate of the funds required for the near future (and this may easily change over time). The algorithm is fairly simple, though it needs explanation. We cannot simply execute eth_call with an overridden contract balance because the gas consumed may be less than what is required during the call (see EIP2200). So, we override the account balance and use binary search to vary feeCredit. A separate issue here is that this process must be done for all messages sent. That is, if we have an external message that leads to an internal message, we must first estimate the fee for the internal message and then for the external message. We do not unpack messages and try not to touch user data. This can be done later. In tests, hardcoded constants were replaced with empty/null values. If the user does not explicitly specify a value, fee estimation is triggered. This allows for more comprehensive code testing. However, it is not possible to completely eliminate this, as we have negative tests that check for errors. It is expected that this will happen asynchronously, while fee estimation is a fully synchronous operation. Enable Github CI for the merge queue Apparently, it's not enough to just enable the merge queue. We also need to make a change to the Actions Workflow, as per https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue#triggering-merge-group-checks-with-github-actions Without this change, merge queue entries will hang indefinitely. Add `IncludedInMain` flag to `RPCReceipt` This flag indicates whether receipt's block is included in the main chain. Relates #800 Signed-off-by: Mikhail Sherstennikov <[email protected]> add tests for minter in hardhat send tokens to another contract fetch endpoint from .env fix tests and improve precompiles copy precompiles everywhere fix tests implement hardhat test for awaitCall add editorconfig to the root for js packages Standard error for missing config keys in nil cli additional Nil.js tests in docs fix to tests Don't needlessly poison build caches. Set version by binary patch I've noticed that every time you commit something locally, the next time you run "nix build", the `nil` derivation always gets rebuilt. This was in particular the case during changing `docs` or `nil.js`, which in theory shouldn't affect `nil` itself. Then I've realized that we pass current revision and commit number as an input to the `nil` derivation, and we embed those into the binary at the build phase. All this means that even though the binary is functionally exactly the same, we will needlessly re-run tests and essentially rebuild everything every time even if we just change something in `hardhat-plugin`. This diff is implementing a simple and straightforward trick of injecting the version number into the binary at the packing phase. At the build phase we just add a pre-defined 40-byte string that can be easily replaced by `sed` (see the `scripts/binary_patch_version.sh` script). By doing so, I'm saving people countless number of needless rebuilds. This should also positively affect the CI as well. Fix timeouts in filters tests Signed-off-by: Mikhail Sherstennikov <[email protected]> Update NotReplaced error to more explicit condition test: fix flaky tests Sometimes (especially locally) some getters tests failed because we didn't have result of deployment/execution committed into main block. add nix/sh formatters Ported from `dbms`. Improve nild config: restructure, rename, add db Remove duplication of npmDeps in JS subprojects Currently there are a few places where you have to update the hash of npmDeps every time it changes. I've moved the fetching of the npm package dependencies to `npmdeps.nix` which removes this duplication and makes things easier to reason about. bump libp2p version Add call + support ABI + args for syncCall/sendCall on WalletV1 tests fix
Closes #947
Also, this patch contains the following changes: - Refined ContractData: - Remove raw compiler output, because it doesn't hold anything useful, except `methodIdentifiers`. - Add `methodIdentifiers`. - Store abi as a json string. - Since Cometa's jsonrpc is quite silly - it just calls same methods from Service, merge it into Service class itself. - Add config file to cometa application - Fix several bugs
Patch for incorrect pubkey length
Remove deploy flag from `asyncCall` method. Now, `asyncDeploy` function should be used for deployment. The reason for this is that asyncCall is too overloaded. For example: - Solidity programmer must always keep in mind that if he wants to use it for deployment, he always must append salt to the bytecode (issue #1138). - Sending tokens is not working when deploy flag is set in asyncCall, this can lead to leak of tokens. Thus, it is better to not mix these two entities, call and deploy, in one function.
fixed possible outofgas errors in tests hash fix removed gas price debug console log remove
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Added detailed contribution guidelines and improved documentation structure to make it easier for new contributors to get started with the project.
Changes
Related Issues
Closes #55
Additional Notes
The contribution guidelines are based on the current project structure and development practices. They should help standardize future contributions and reduce the maintainers' review workload.