App abstraction (go changes) #2349
Merged
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
WalkthroughThe recent updates encompass modifications across various components of the SIN executor, focusing on bytecode updates in contract metadata, renaming and path adjustments in mock contracts and generator files, and functional changes in test utilities and executor tests. These changes streamline contract deployment, enhance testing capabilities, and refine the project's structure for better clarity and efficiency. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yaml
Files selected for processing (10)
- sin-executor/contracts/executionservice/executionservice.abigen.go (2 hunks)
- sin-executor/contracts/interchainclient/interchainclient.abigen.go (7 hunks)
- sin-executor/contracts/mocks/executionfeesmock/executionfeesmock.abigen.go (1 hunks)
- sin-executor/contracts/mocks/executionfeesmock/executionfeesmock.contractinfo.json (1 hunks)
- sin-executor/contracts/mocks/executionfeesmock/generate.go (1 hunks)
- sin-executor/contracts/mocks/interchainapp/generate.go (1 hunks)
- sin-executor/contracts/mocks/interchainapp/helpers.go (2 hunks)
- sin-executor/executor/executor_test.go (1 hunks)
- sin-executor/testutil/contracttype.go (1 hunks)
- sin-executor/testutil/deployers.go (1 hunks)
Files not summarized due to errors (2)
- sin-executor/contracts/interchainclient/interchainclient.abigen.go: Error: Message exceeds token limit
- sin-executor/contracts/mocks/executionfeesmock/executionfeesmock.contractinfo.json: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- sin-executor/contracts/mocks/executionfeesmock/generate.go
Additional comments: 16
sin-executor/executor/executor_test.go (1)
- 38-38: The addition of
big.NewInt(0)as an argument toSendMessagealigns with the PR objectives. Ensure that all calls toSendMessageacross the codebase have been updated to match the new signature.sin-executor/testutil/contracttype.go (2)
- 101-101: The update to the contract reference for
InterchainAppto point toExampleAppV1.sol:ExampleAppV1aligns with the PR's renaming and rebranding efforts.- 107-107: Ensure the updated reference for
ExecutionFeesMockaligns with the changes in contract structures or functionalities.sin-executor/testutil/deployers.go (2)
- 88-88: The method for deploying the interchain app has been updated to use
DeployExampleAppV1, aligning with the PR's objectives.- 94-94: The method for setting the interchain client has been updated, ensuring it aligns with the updated functionalities.
sin-executor/contracts/mocks/executionfeesmock/executionfeesmock.contractinfo.json (1)
- 1-1: The JSON structure appears well-formed and includes detailed information about the
ExecutionFeesMockcontract and theIExecutionFeesinterface. It's important to ensure that thecompilerVersion,compilerOptions, andevmVersionspecified in theinfosection align with the project's standards and the Solidity version used in other contracts within the project. Additionally, verify that themetadatasection'sbytecodeHashsetting ("ipfs") is consistent with how bytecode hashes are managed across the project. If the project uses a different platform or protocol for bytecode verification, this setting should be adjusted accordingly.sin-executor/contracts/mocks/executionfeesmock/executionfeesmock.abigen.go (2)
- 44-44: The
Binfield inExecutionFeesMockMetaDatahas been updated. While the bytecode itself cannot be directly reviewed for correctness, it's important to ensure that this update corresponds to the intended version of the contract. Please verify the update process was correctly followed and consider updating any related documentation to reflect this change.- 44-44: The generated code for the
ExecutionFeesMockcontract binding follows best practices for Ethereum contract bindings in Go. Note the use of deprecated fields (ExecutionFeesMockABI,ExecutionFeesMockFuncSigs,ExecutionFeesMockBin) is common in generated bindings to maintain backward compatibility.sin-executor/contracts/executionservice/executionservice.abigen.go (1)
- 199-199: The
Binfield forExecutionServiceMetaDatais updated. Ensure that the new bytecode corresponds to the latest compiled version of the smart contract. This is crucial for the correctness and security of interactions with the blockchain.sin-executor/contracts/interchainclient/interchainclient.abigen.go (7)
- 76-76: The
Binfield forAppConfigLibMetaDatahas been updated. Ensure that the new bytecode corresponds to the latest compiled version of theAppConfigLibcontract and that it has been thoroughly tested, especially if the contract's logic has changed.- 2279-2279: The
Binfield forInterchainBatchLibMetaDatahas been updated. Similar to the previous comment, verify that this bytecode update reflects the latest changes and optimizations in theInterchainBatchLibcontract and that comprehensive testing has been conducted.- 2473-2473: Updates to the
Binfield for a contract are noted without specifying the contract's name. Ensure that this bytecode update is correctly associated with the intended contract and that the contract's functionality aligns with these changes.- 4995-4995: The
Binfield forInterchainEntryLibMetaDatahas been updated. As with the other contracts, confirm that this update accurately represents the latest compiled bytecode and that the contract has been adequately tested post-update.- 5168-5168: The
Binfield forInterchainTransactionLibMetaDatahas been updated. It's crucial to ensure that this bytecode is the result of the latest contract compilation and that the contract's functionality has been validated through testing.- 5341-5341: The
Binfield forOptionsLibMetaDatahas been updated. Verify that this update is reflective of the most recent changes to theOptionsLibcontract and that the contract has undergone thorough testing to ensure its functionality.- 5900-5900: The
Binfield forTypeCastsMetaDatahas been updated. Ensure that this bytecode corresponds to the latest version of theTypeCastscontract and that the contract has been tested extensively to confirm its functionality.
| @@ -9,8 +9,8 @@ | |||
| ) | |||
|
|
|||
| // InterchainAppMockRef is a reference to an interchain db. | |||
| type InterchainAppMockRef struct { | |||
There was a problem hiding this comment.
Consider renaming InterchainAppMockRef to MockRef to avoid stuttering in the package name usage, as suggested by Go conventions.
- type InterchainAppMockRef struct {
+ type MockRef struct {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| type InterchainAppMockRef struct { | |
| type MockRef struct { |
Comment on lines
85
to
97
| clientContract := helpers.Registry().Get(ctx, InterchainClient) | ||
| sendingModule := helpers.Registry().Get(ctx, InterchainModuleMock) | ||
|
|
||
| appAddress, appTx, appMock, err := interchainapp.DeployInterchainAppExample(transactOps, backend, transactOps.From) | ||
| appAddress, appTx, appMock, err := interchainapp.DeployExampleAppV1(transactOps, backend, transactOps.From) | ||
| if err != nil { | ||
| return common.Address{}, nil, nil, fmt.Errorf("could not deploy interchain app mock: %w", err) | ||
| } | ||
| helpers.Backend().WaitForConfirmation(ctx, appTx) | ||
|
|
||
| tx, err := appMock.SetInterchainClient(transactOps, clientContract.Address()) | ||
| tx, err := appMock.AddInterchainClient(transactOps, clientContract.Address(), true) | ||
| if err != nil { | ||
| return common.Address{}, nil, nil, fmt.Errorf("could not set interchain client: %w", err) | ||
| } |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [78-78]
Consider removing or renaming unused parameters in functions to _ to adhere to Go best practices.
- func(ctx context.Context, helpers deployer.IFunctionalDeployer, transactOps *bind.TransactOpts, backend bind.ContractBackend) (common.Address, *types.Transaction, interface{}, error) {
+ func(_ context.Context, helpers deployer.IFunctionalDeployer, transactOps *bind.TransactOpts, backend bind.ContractBackend) (common.Address, *types.Transaction, interface{}, error) {Also applies to: 111-111, 141-141
Deploying sanguine-fe with
|
| Latest commit: |
4a14231
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0fd729d3.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-interchain-app-abstract-kqxl.sanguine-fe.pages.dev |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- sin-executor/executor/suite_test.go (1 hunks)
Additional comments: 1
sin-executor/executor/suite_test.go (1)
- 121-121: Changing the
OptimisticPeriodfrom0to1in thesetClientConfigsmethod affects the configuration setup for the application. This change likely impacts how optimistic operations are simulated or tested within this suite. Ensure that this adjustment aligns with the intended behavior of the tests and consider if any related documentation or other test configurations need to be updated to reflect this change.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feat/interchain-app-abstraction #2349 +/- ##
=========================================================================
+ Coverage 46.85358% 46.86729% +0.01370%
=========================================================================
Files 383 383
Lines 29176 29176
Branches 83 83
=========================================================================
+ Hits 13670 13674 +4
+ Misses 14048 14044 -4
Partials 1458 1458
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- sin-executor/executor/executor_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- sin-executor/executor/executor_test.go
ChiTimesChi
merged commit 2f21dcb
into
feat/interchain-app-abstraction
28 of 29 checks passed
ChiTimesChi
added a commit
that referenced
this pull request
Mar 27, 2024
* Update `generate.go` * Regenerate `sin-executor` * Update Example App references * Use non-zero period in tests * Use `GetMessageFee` for setting the message value * Use non-zero gas airdrop amount in tests
ChiTimesChi
added a commit
that referenced
this pull request
Mar 27, 2024
* Abstraction for receiving messages * Abstraction for sending messages * Fully implement IInterchainApp * Abstraction for managing clients * Scaffold ICAppV1 * ICAppV1: implement storage * ICAppV1: implement permissioned management * Add management views * Add thin wrapper for "send message to linked app" * Add more custom errors * Add App harness and client mock * Add ICAppV1 management tests * Add extra checks for the new custom errors * Chore: move stuff around in tests * Fix: IInterchainAppV1 should implement IInterchainApp * Add ICAppV1 messaging tests * Add `_getMessageFee` thin wrapper and tests around it * Update PingPongApp to use the `ICAppV1` base * Update ExampleApp * Move example apps into a separate folder * Update scripts and tests * Chore: cleanup * Update flattened files set [**.sol glob not working] * Add fee getter for `ExampleAppV1` * App abstraction (go changes) (#2349) * Update `generate.go` * Regenerate `sin-executor` * Update Example App references * Use non-zero period in tests * Use `GetMessageFee` for setting the message value * Use non-zero gas airdrop amount in tests * Regenerate `sin-executor` * Fix: incorrect fuzzing "not IC client" test
golangisfun123
pushed a commit
that referenced
this pull request
Jun 19, 2024
* Update `generate.go` * Regenerate `sin-executor` * Update Example App references * Use non-zero period in tests * Use `GetMessageFee` for setting the message value * Use non-zero gas airdrop amount in tests
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
A clear and concise description of the features you're adding in this pull request.
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit