Crescendo updates for Rosetta#63
Conversation
…nd for deploying contract
|
Warning Rate limit exceeded@franklywatson has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 13 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis update involves several enhancements and adjustments across the codebase. The Makefile has seen orchestration changes, while the README and various Go and Cadence files have been updated to support the new Changes
Poem
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 Configuration File (
|
franklywatson
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- script/cadence/contracts/FlowColdStorageProxy.cdc (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- script/cadence/contracts/FlowColdStorageProxy.cdc
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- script/cadence/contracts/FlowColdStorageProxy.cdc (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- script/cadence/contracts/FlowColdStorageProxy.cdc
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- script/cadence/contracts/FlowColdStorageProxy.cdc (8 hunks)
- script/cadence/scripts/get-balances-basic.cdc (2 hunks)
- script/cadence/scripts/get-balances.cdc (2 hunks)
- script/cadence/transactions/basic-transfer.cdc (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- script/cadence/contracts/FlowColdStorageProxy.cdc
- script/cadence/transactions/basic-transfer.cdc
Additional comments not posted (6)
script/cadence/scripts/get-balances-basic.cdc (3)
4-7: Confirm the intended restriction of access.Changing the access control from
pubtoaccess(all)restricts the visibility of theAccountBalancesstruct to the current module. Ensure this is the intended behavior.
16-16: Confirm the intended restriction of access.Changing the access control from
pubtoaccess(all)restricts the visibility of themainfunction to the current module. Ensure this is the intended behavior.
18-21: LGTM! Improved reliability by checkingbalanceRefexistence.The new code correctly checks for the existence of
balanceRefbefore accessing its balance, preventing potential runtime errors.script/cadence/scripts/get-balances.cdc (3)
5-5: Confirm the intended restriction of access.Changing the access control from
pubtoaccess(all)restricts the visibility of theAccountBalancesstruct to the current module. Ensure this is the intended behavior.
17-17: Confirm the intended restriction of access.Changing the access control from
pubtoaccess(all)restricts the visibility of themainfunction to the current module. Ensure this is the intended behavior.
19-22: LGTM! Improved reliability by checkingbalanceRefandrefexistence.The new code correctly checks for the existence of
balanceRefandrefbefore accessing their balances, preventing potential runtime errors.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- access/access.go (19 hunks)
- config/config.go (4 hunks)
- script/flow.json (1 hunks)
Additional comments not posted (18)
script/flow.json (1)
13-13: LGTM!The new network configuration for "previewnet" is correctly added.
config/config.go (4)
192-192: LGTM!The added log message for reading the config file is useful for debugging and monitoring.
211-211: LGTM!The network validation logic correctly includes "previewnet" as a valid network.
388-389: LGTM!The added log message for accessing nodes config is useful for debugging and monitoring.
399-399: LGTM!The version validation range is correctly adjusted to accommodate version 7.
access/access.go (13)
15-17: LGTM!The added imports for OpenTelemetry attributes and metrics are necessary for the added logging and tracing functionality.
23-23: LGTM!The updated import paths for crypto packages are necessary to align with the new package structure.
92-92: LGTM!The added logging statement in
Client.Accountis useful for debugging and monitoring.
119-119: LGTM!The added logging statement in
Client.AccountAtHeightis useful for debugging and monitoring.
140-140: LGTM!The added logging statement in
Client.BlockByHeightis useful for debugging and monitoring.
161-161: LGTM!The added logging statement in
Client.BlockByIDis useful for debugging and monitoring.
188-188: LGTM!The added logging statement in
Client.BlockEventsis useful for debugging and monitoring.
209-209: LGTM!The added logging statement in
Client.BlockHeaderByHeightis useful for debugging and monitoring.
229-229: LGTM!The added logging statement in
Client.BlockHeaderByIDis useful for debugging and monitoring.
250-250: LGTM!The added logging statement in
Client.CollectionByIDis useful for debugging and monitoring.
294-294: LGTM!The added logging statement in
Client.Executeis useful for debugging and monitoring.
314-314: LGTM!The added logging statement in
Client.ExecutionResultForBlockIDis useful for debugging and monitoring.
336-336: LGTM!The added logging statement in
Client.LatestBlockHeaderis useful for debugging and monitoring.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
previewnet.json (1)
22-22: Inconsistency inspork_seal_tolerancevalues across environmentsThe
spork_seal_tolerancevalue is set to10000inpreviewnet.jsonandlocalnet.json, while it is set to600intestnet.json,testnet-clone.json, andmainnet.json. This discrepancy needs to be addressed to ensure consistency and appropriateness for the use case.
previewnet.json:10000localnet.json:10000testnet.json:600testnet-clone.json:600mainnet.json:600Please verify the tolerance value and ensure it is consistent and appropriate across all environments.
Analysis chain
LGTM! Spork seal tolerance correctly specified.
The
spork_seal_tolerancesetting is correctly specified as10000.However, ensure that the tolerance value is appropriate for the use case.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the spork seal tolerance value. # Test: Ensure the spork seal tolerance value is appropriate for the use case. rg --type json -A 5 $'spork_seal_tolerance'Length of output: 1355
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- previewnet.json (1 hunks)
Additional comments not posted (12)
previewnet.json (12)
2-2: LGTM! Cache setting enabled.The
cachesetting is enabled, which is beneficial for performance.
3-7: LGTM! Construction access nodes correctly specified.The
construction_access_nodessetting contains the correct access node address for the PreviewNet network.
15-15: LGTM! Data directory correctly specified.The
data_dirsetting is set to./data, which is appropriate for storing data.
16-16: LGTM! Consensus follower disabled.The
disable_consensus_followersetting is enabled, which is appropriate if consensus following is not required.
17-17: LGTM! Network name correctly specified.The
networksetting is correctly specified aspreviewnet.
26-30: LGTM! Access nodes within sporks correctly specified.The
access_nodessetting withinsporkscontains the correct access node address for the PreviewNet network.
31-33: LGTM! Root block and version within sporks correctly specified.The
root_blockandversionsettings withinsporksare correctly specified.However, ensure that the root block number and version are accurate and up-to-date.
Verification successful
Verified: Root block and version within sporks are correctly specified.
The
root_blockandversionsettings for the PreviewNet network inpreviewnet.jsonare accurate and up-to-date.
root_block: 14700328version: 7Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the root block number and version for the PreviewNet network. # Test: Ensure the root block number and version are accurate and up-to-date. rg --type json -A 5 $'root_block'Length of output: 1026
8-14: LGTM! Contracts correctly specified.The
contractssetting contains the correct contract addresses for the PreviewNet network.However, ensure that the contract addresses are accurate and up-to-date.
18-20: LGTM! Originators correctly specified.The
originatorssetting contains the correct originator address.However, ensure that the originator address is accurate and up-to-date.
24-34: LGTM! Sporks correctly specified.The
sporkssetting contains the correct spork configurations for the PreviewNet network.However, ensure that the spork configurations are accurate and up-to-date.
23-23: LGTM! Spork synced tolerance correctly specified.The
spork_synced_tolerancesetting is correctly specified as10.However, ensure that the tolerance value is appropriate for the use case.
21-21: LGTM! Port number correctly specified.The
portsetting is correctly specified as8080, which is a common choice for web services.However, ensure that the port number does not conflict with other services.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- script/cadence/transactions/proxy-to-flow-transfer.cdc (2 hunks)
Additional context used
Learnings (1)
script/cadence/transactions/proxy-to-flow-transfer.cdc (1)
Learnt from: franklywatson PR: onflow/rosetta#63 File: script/script.go:23-23 Timestamp: 2024-07-16T15:22:29.703Z Learning: The syntax `let xfer: @{FungibleToken.Vault}` is considered correct in the context of Cadence 1.0 as per user franklywatson.
Additional comments not posted (4)
script/cadence/transactions/proxy-to-flow-transfer.cdc (4)
8-8: LGTM! Correct syntax for Cadence 1.0.The change to use curly braces around
FungibleToken.Vaultis correct.
10-10: LGTM! Correct syntax for Cadence 1.0.The change to use
auth(BorrowValue) &Accountis correct.
12-12: LGTM! Correct syntax for Cadence 1.0.The change to include
auth(FungibleToken.Withdraw)is correct.However, ensure that all function calls to
sender.storage.borrowmatch the new syntax.Verification successful
Verification Successful: All occurrences of
sender.storage.borrowmatch the new syntax.The change to include
auth(FungibleToken.Withdraw)is consistently applied across the codebase.
script/script.goscript/cadence/transactions/proxy-to-flow-transfer.cdcscript/cadence/transactions/basic-transfer.cdcScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `sender.storage.borrow` match the new syntax. # Test: Search for the function usage. Expect: Only occurrences of the new syntax. rg --type cadence -A 5 $'sender.storage.borrow<auth(FungibleToken.Withdraw)'Length of output: 109
Script:
#!/bin/bash # Description: Verify all function calls to `sender.storage.borrow` match the new syntax. # Test: Search for the function usage. Expect: Only occurrences of the new syntax. rg -A 5 $'sender.storage.borrow<auth(FungibleToken.Withdraw)'Length of output: 62
Script:
#!/bin/bash # Description: Verify all function calls to `sender.storage.borrow` match the new syntax. # Test: Search for all occurrences of `sender.storage.borrow` in the codebase. rg 'sender.storage.borrow'Length of output: 505
22-22: LGTM! Correct syntax for Cadence 1.0.The change to use
capabilitiesinstead ofgetCapabilityis correct.However, ensure that all function calls to
getAccount(receiver).capabilities.borrowmatch the new syntax.Verification successful
Verification Successful: The new syntax is consistently used.
The change to use
capabilitiesinstead ofgetCapabilityis correctly implemented across the codebase.
README.mdscript/script.goscript/cadence/transactions/proxy-to-flow-transfer.cdcscript/cadence/transactions/basic-transfer.cdcscript/cadence/contracts/FlowColdStorageProxy.cdcScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `getAccount(receiver).capabilities.borrow` match the new syntax. # Test: Search for the function usage. Expect: Only occurrences of the new syntax. rg --type cadence -A 5 $'getAccount(receiver).capabilities.borrow'Length of output: 99
Script:
#!/bin/bash # Description: Verify all function calls to `getAccount(receiver).capabilities.borrow` match the new syntax. # Test: Search for the function usage. Expect: Only occurrences of the new syntax. rg -A 5 'getAccount(receiver).capabilities.borrow'Length of output: 52
Script:
#!/bin/bash # Description: Verify all function calls to `getAccount(receiver)` and `borrow` match the new syntax. # Test: Search for the function usage. Expect: Only occurrences of the new syntax. rg -A 5 'getAccount(receiver)' rg -A 5 'borrow<&{FungibleToken.Receiver}>'Length of output: 203
Script:
#!/bin/bash # Description: Verify all function calls to `getAccount(receiver)` and `borrow` match the new syntax. # Test: Search for the function usage. Expect: Only occurrences of the new syntax. rg -A 5 'getAccount(receiver)' rg -A 5 'borrow<&\{FungibleToken\.Receiver\}>'Length of output: 2501
sisyphusSmiling
left a comment
sisyphusSmiling
left a comment
There was a problem hiding this comment.
Great work! Left some small suggestions and comments
Co-authored-by: Giovanni Sanchez <108043524+sisyphusSmiling@users.noreply.github.com>
Co-authored-by: Giovanni Sanchez <108043524+sisyphusSmiling@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- script/cadence/scripts/get-public-key.cdc (1 hunks)
- script/cadence/scripts/get-revoke-status.cdc (1 hunks)
Additional comments not posted (4)
script/cadence/scripts/get-revoke-status.cdc (2)
1-1: Access control change looks good.Changing
pubtoaccess(all)is appropriate for this context.
4-4: Error handling improvement looks good.The added error handling ensures that the function panics with a descriptive message if the key is not found.
script/cadence/scripts/get-public-key.cdc (2)
1-1: Access control change looks good.Changing
pubtoaccess(all)is appropriate for this context.
4-4: Error handling improvement looks good.The added error handling ensures that the function panics with a descriptive message if the key is not found.
3 participants
Summary by CodeRabbit
New Features
previewnetnetwork configuration for enhanced network options.previewnet-integration-testtarget in build scripts.Improvements
Refactor
relictargets and tags from build and linting processes.