[Rosetta] Test updates required for Crescendo #59
Conversation
|
@peterargue, can you review it please? |
WalkthroughThe recent changes to the Cadence transaction scripts and Rosetta integration primarily involve refining resource borrowing, updating capability handling, and modifying authorization types. These adjustments ensure compatibility with the Crescendo upgrade and address payload hash mismatches for the latest Flow-go version. Changes
Sequence Diagram(s)No sequence diagrams are provided as the changes are too varied and simple to warrant visual representation. Assessment against linked issues
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 (
|
…nd for deploying contract
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (29)
script/script.go (1)
23-23: Consider using a more descriptive variable name thanxfer.A more descriptive name could improve code readability. For example,
transferVaultmight be more informative.api/call_service.go (1)
135-135: Improve error handling by providing more context in the error message.Consider including additional details in the error message to aid in debugging, such as the expected type.
integration_test.py (3)
Line range hint
3-3: Remove unused import:threading.Thread.- from threading import Thread
Line range hint
4-4: Remove unused import:os.- import os
149-149: Address the unused variableresult.Consider using the variable or remove it if not needed.
state/process.go (3)
Line range hint
435-539: The error handling and logging within the event decoding and field extraction are robust, ensuring that any issues during transaction processing are logged and appropriately handled. However, consider refactoring to reduce complexity and improve readability.- if err != nil { - log.Errorf( - "Unable to hex decode public key from FlowColdStorageProxy.Created event in transaction %x in block %x at height %d: %s", - txnHash, hash, height, err, - ) - skipCache = true - continue outer - } + handleEventError(err, "Unable to hex decode public key from FlowColdStorageProxy.Created event", txnHash, hash, height, &skipCache)Create a helper function
handleEventErrorto handle repetitive error logging and state modification.
Line range hint
565-683: The handling ofFlowColdStorageProxy.DepositedandFlowColdStorageProxy.Transferredevents is consistent with the rest of the function. However, the repeated pattern of error handling and field extraction suggests that this could be abstracted into a utility function to reduce duplication and improve maintainability.- if err != nil { - log.Errorf( - "Unable to load the `amount` from FlowColdStorageProxy.Transferred event in transaction %x in block %x at height %d", - txnHash, hash, height, - ) - skipCache = true - continue outer - } + handleEventError(err, "Unable to load the `amount` from FlowColdStorageProxy.Transferred event", txnHash, hash, height, &skipCache)Again, use the proposed
handleEventErrorfunction to streamline error handling.
Line range hint
712-834: The processing ofFlowToken.TokensDepositedandFlowToken.TokensWithdrawnevents is handled correctly. However, the complexity and repetitive patterns in event handling could be reduced by abstracting common logic into helper functions.- if err != nil { - log.Errorf( - "Unable to load sender address from FlowToken.TokensWithdrawn event in transaction %x in block %x at height %d", - txnHash, hash, height, - ) - skipCache = true - continue outer - } + handleEventError(err, "Unable to load sender address from FlowToken.TokensWithdrawn event", txnHash, hash, height, &skipCache)This refactoring would make the code cleaner and more maintainable by reducing duplication.
api/construction_service.go (1)
Line range hint
1139-1155: Refactor repeated pattern indecodeContractOps.The function
decodeContractOpscontains multiple instances where a pattern of decoding and type assertion is repeated. This can be refactored into a helper function to reduce code duplication and improve maintainability. Here's a proposed helper function:func decodeCadenceString(arg []byte) (string, error) { raw, err := jsoncdc.Decode(access.NoopMemoryGauge, arg) if err != nil { return "", fmt.Errorf("unable to decode transaction arg: %s", err) } result, ok := raw.(cadence.String) if !ok { return "", fmt.Errorf("unable to convert transaction arg to string") } return string(result), nil }You can then replace the repeated blocks with calls to this helper function, simplifying the
decodeContractOpsfunction.Also applies to: 1167-1179, 1191-1197, 1219-1231, 1237-1251, 1266-1266
README.md (20)
Line range hint
135-135: Consider using "among" instead of "amongst" for a more modern tone.
Line range hint
184-184: Consider simplifying "In order to support" to "To support" for conciseness.- In order to support balance reconciliation by `rosetta-cli check:data`, we + To support balance reconciliation by `rosetta-cli check:data`, we
Line range hint
300-300: Consider adding a comma after "production systems" for clarity.- This can be used to enable the caching of idempotent Access API calls. This should generally be set to `true` in production systems as it makes a + This can be used to enable the caching of idempotent Access API calls. This should generally be set to `true` in production systems, as it makes a
Line range hint
410-410: Consider adding a comma after "risk" for clarity.- This is a security risk as a compromised Access API server could + This is a security risk, as a compromised Access API server could
Line range hint
462-462: Consider adding a comma after "follower".- within the config for the current spork unless `disable_consensus_follower` + within the config for the current spork, unless `disable_consensus_follower`
Line range hint
470-470: Consider using "to" instead of "so as to" for a more direct and modern tone.- This is so as to enable spinning up a new Flow Rosetta node + This is to enable spinning up a new Flow Rosetta node
Line range hint
500-500: Consider rephrasing to avoid repetitive sentence beginnings.- * This defines the HTTPS URL for the file containing the JSON root protocol state snapshot data for the live spork. + * Defines the HTTPS URL for the file containing the JSON root protocol state snapshot data for the live spork.
Line range hint
518-518: Correct the verb form after "to" to its base form.- to signed the root protocol state snapshot data + to sign the root protocol state snapshot data
Line range hint
541-541: Consider simplifying "In order to support" to "To support" for conciseness.- In order to support such cold storage transactions, we + To support such cold storage transactions, we
Line range hint
582-582: Consider using "to" instead of "so as to" for a more direct and modern tone.- so as to make their storage immutable except through + to make their storage immutable except through
Line range hint
592-592: Correct the verb form to match the singular subject "minimum".- if the minimum account balance ever increases, this can be use + if the minimum account balance ever increases, this can be used
Line range hint
678-678: Consider removing the comma after "accidentally".- If you accidentally, enabled support for proxy + If you accidentally enabled support for proxy
Line range hint
702-702: Consider removing "outside of" for conciseness.- outside of the `operations`. + the `operations`.
Line range hint
809-809: Consider using "deployment" instead of "deploy" for clarity.- This key will be revoked as part of the deploy transaction. + This key will be revoked as part of the deployment transaction.
Line range hint
862-862: Consider using "same" instead of "exact same" for conciseness.- the exact same account as the `payer`. + the same account as the `payer`.
Line range hint
1094-1094: Add an article before "first".- first is the hex-encoded private key, and the + the first is the hex-encoded private key, and the
Line range hint
1363-1363: Consider using "same" instead of "exact same" for conciseness.- Proxy accounts can be created in the exact same way as normal accounts + Proxy accounts can be created in the same way as normal accounts
Line range hint
1444-1444: Correct the verb form to "follow up".- If we follow-up with a request to `/construction/payloads` + If we follow up with a request to `/construction/payloads`
Line range hint
2108-2108: Consider rephrasing for clarity.- just using the root originator in this example as it's an account we control which has + using the root originator in this example because it's an account we control that has
Line range hint
2112-2112: Consider simplifying "in order to" to "to" for conciseness.- in order to construct the "inner transaction": + to construct the "inner transaction":
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum
Files selected for processing (28)
- Makefile (2 hunks)
- README.md (1 hunks)
- access/access.go (1 hunks)
- api/account_service.go (1 hunks)
- api/call_service.go (4 hunks)
- api/construction_service.go (21 hunks)
- cmd/corecontracts/corecontracts.go (1 hunks)
- cmd/tipheight/tipheight.go (1 hunks)
- cmd/txinfo/txinfo.go (1 hunks)
- config/config.go (2 hunks)
- integration_test.py (8 hunks)
- localnet.json (3 hunks)
- previewnet.json (1 hunks)
- previewnet_integration_test.py (1 hunks)
- script/cadence/scripts/get-active-keys-length.cdc (1 hunks)
- script/cadence/scripts/get-balances-basic.cdc (2 hunks)
- script/cadence/scripts/get-key-index.cdc (1 hunks)
- script/cadence/scripts/get-keys-length.cdc (1 hunks)
- script/cadence/scripts/get-proxy-newmethod.cdc (1 hunks)
- script/cadence/scripts/get-proxy-nonce.cdc (1 hunks)
- script/cadence/scripts/get-public-key.cdc (1 hunks)
- script/cadence/scripts/get-revoke-status.cdc (1 hunks)
- script/cadence/transactions/basic-transfer.cdc (2 hunks)
- script/script.go (9 hunks)
- state/convert.go (7 hunks)
- state/process.go (23 hunks)
- state/state.go (1 hunks)
- testnet.json (1 hunks)
Files not reviewed due to errors (1)
- config/config.go (no review received)
Files skipped from review due to trivial changes (5)
- cmd/corecontracts/corecontracts.go
- cmd/tipheight/tipheight.go
- previewnet.json
- script/cadence/scripts/get-public-key.cdc
- testnet.json
Additional Context Used
LanguageTool (21)
README.md (21)
Near line 135: The preposition ‘amongst’ is correct, but some people think that it is old-fashioned or literary. A more frequently used alternative is the preposition “among”.
Context: ...the main command line binaries. Primary amongst these is theservercommand. * `co...
Rule ID: AMONGST
Near line 184: Consider a shorter alternative to avoid wordiness.
Context: ...atch the on-chain balance changes. * In order to support balance reconciliation by `rose...
Rule ID: IN_ORDER_TO_PREMIUM
Near line 300: Possible missing comma found.
Context: ...enerally be set totruein production systems as it makes a massive difference in...
Rule ID: AI_HYDRA_LEO_MISSING_COMMA
Near line 410: Possible missing comma found.
Context: ...previous spork. * This is a security risk as a compromised Access API server coul...
Rule ID: AI_HYDRA_LEO_MISSING_COMMA
Near line 462: Possible missing comma found.
Context: ...d within the config for the current spork unlessdisable_consensus_follower...
Rule ID: AI_HYDRA_LEO_MISSING_COMMA
Near line 470: ‘So as to’ expresses purpose and is used in formal texts. Consider using “to”.
Context: ...ny block within that spork. This is so as to enable spinning up a new Flow Rosetta n...
Rule ID: SO_AS_TO
Near line 500: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...root_protocol_state_url: string* This defines the HTTPS URL for the file cont...
Rule ID: ENGLISH_WORD_REPEAT_BEGINNING_RULE
Near line 518: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive.
Context: ...contents of the armored PGP key used to signed the root protocol state snapshot da...
Rule ID: TO_NON_BASE
Near line 541: Consider a shorter alternative to avoid wordiness.
Context: ...en take longer than the expiry window. In order to support such cold storage transactions,...
Rule ID: IN_ORDER_TO_PREMIUM
Near line 582: ‘So as to’ expresses purpose and is used in formal texts. Consider using “to”.
Context: ...e accounts are created without any keys so as to make their storage immutable except thr...
Rule ID: SO_AS_TO
Near line 592: “Minimum” is a singular noun. It appears that the verb form is incorrect.
Context: ...okenReceiver` so that if the minimum account balance ever increases, this can be use...
Rule ID: PCT_SINGULAR_NOUN_PLURAL_VERB_AGREEMENT
Near line 678: Possible missing comma found.
Context: ...y account support will be disabled. If you accidentally, enabled support for proxy...
Rule ID: AI_HYDRA_LEO_MISSING_COMMA
Near line 702: This phrase is redundant. Consider using “outside”.
Context: ...op-level requestmetadatafield, i.e. outside of theoperations. The nonce/sequence n...
Rule ID: OUTSIDE_OF
Near line 809: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...This key will be revoked as part of the deploy transaction. Note: Once a `deploy_cont...
Rule ID: PREPOSITION_VERB
Near line 862: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ... the "sender" of atransferto be the exact same account as thepayer. ### Proxy Tran...
Rule ID: EN_WORDINESS_PREMIUM_EXACT_SAME
Near line 1094: Possible missing article found.
Context: ...1d64f886a ``` It takes two parameters, first is the hex-encoded private key, and the...
Rule ID: AI_HYDRA_LEO_MISSING_THE
Near line 1363: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...I Proxy accounts can be created in the exact same way as normal accounts, just use the op...
Rule ID: EN_WORDINESS_PREMIUM_EXACT_SAME
Near line 1444: The word “follow-up” is a noun. The verb is spelled with a space.
Context: ...value": "100648" } ] } ``` If we follow-up with a request to `/construction/payloa...
Rule ID: NOUN_VERB_CONFUSION
Near line 2108: Possible missing comma found.
Context: ... just using the root originator in this example as it's an account we control which has...
Rule ID: AI_HYDRA_LEO_MISSING_COMMA
Near line 2111: Consider a more expressive alternative.
Context: ...nt we control which has some funds. To do this, we first call `/construction/prep...
Rule ID: DO_ACHIEVE
Near line 2112: Consider a shorter alternative to avoid wordiness.
Context: ...peration type ofproxy_transfer_innerin order to construct the "inner transaction": ```...
Rule ID: IN_ORDER_TO_PREMIUM
Ruff (3)
integration_test.py (3)
3-3:
threading.Threadimported but unused
4-4:
osimported but unused
149-149: Local variable
resultis assigned to but never used
Markdownlint (23)
README.md (23)
331: Expected: 2; Actual: 0
Inconsistent indentation for list items at the same level
336: Expected: 2; Actual: 0
Inconsistent indentation for list items at the same level
341: Expected: 2; Actual: 0
Inconsistent indentation for list items at the same level
348: Expected: 2; Actual: 0
Inconsistent indentation for list items at the same level
355: Expected: 2; Actual: 0
Inconsistent indentation for list items at the same level
361: Expected: 2; Actual: 0
Inconsistent indentation for list items at the same level
365: Expected: 2; Actual: 0
Inconsistent indentation for list items at the same level
374: Expected: 2; Actual: 0
Inconsistent indentation for list items at the same level
378: Expected: 2; Actual: 0
Inconsistent indentation for list items at the same level
388: Expected: 2; Actual: 0
Inconsistent indentation for list items at the same level
395: Expected: 2; Actual: 0
Inconsistent indentation for list items at the same level
403: Expected: 2; Actual: 0
Inconsistent indentation for list items at the same level
414: Expected: 2; Actual: 0
Inconsistent indentation for list items at the same level
424: Expected: 2; Actual: 0
Inconsistent indentation for list items at the same level
326: Expected: 0; Actual: 2
Unordered list indentation
800: null
Dollar signs used before commands without showing output
1211: null
Bare URL used
1557: null
Bare URL used
1907: null
Bare URL used
2572: null
Bare URL used
5: null
Fenced code blocks should have a language specified
236: null
Fenced code blocks should have a language specified
2769: Unused link or image reference definition: "access api"
Link and image reference definitions should be needed
Additional comments not posted (43)
script/cadence/scripts/get-key-index.cdc (1)
1-1: Visibility change frompubtoaccess(all)aligns with the PR objectives to enhance access control.script/cadence/scripts/get-revoke-status.cdc (1)
1-1: Visibility change frompubtoaccess(all)is consistent with the PR's objectives to refine access controls.script/cadence/scripts/get-keys-length.cdc (1)
1-1: Visibility change frompubtoaccess(all)aligns with the PR's objectives to enhance access control.script/cadence/scripts/get-proxy-newmethod.cdc (1)
3-3: Visibility change frompubtoaccess(all)is consistent with the PR's objectives to refine access controls.script/cadence/scripts/get-proxy-nonce.cdc (1)
3-3: Visibility change frompubtoaccess(all)aligns with the PR's objectives to enhance access control.script/cadence/scripts/get-active-keys-length.cdc (1)
1-1: Visibility change frompubtoaccess(all)is consistent with the PR's objectives to refine access controls.Makefile (3)
6-6: The update to thego-buildtarget looks good.
15-15: The retention ofstaticcheckin thelinttarget aligns with the previous discussion and is a good practice for static code analysis.
31-32: The addition of thepreviewnet-integration-testtarget is a valuable update for ensuring the application functions correctly in the new network environment.localnet.json (2)
5-5: Updating theaddressunderconstruction_access_nodesto127.0.0.1:4001is appropriate for local network configurations.
16-16: Setting thenetworkfield tolocalnetenhances clarity and helps in distinguishing different network environments in configuration.script/cadence/scripts/get-balances-basic.cdc (2)
4-7: Changing the visibility of theAccountBalancesstruct and its properties toaccess(all)is appropriate if broader access is required for integration or testing.
16-16: The update to themainfunction's visibility toaccess(all)aligns with the changes to theAccountBalancesstruct, ensuring consistency across the script.script/cadence/transactions/basic-transfer.cdc (3)
7-7: Explicitly typing thexfervariable as@{FungibleToken.Vault}enhances type safety and clarity in the transaction script.
9-11: The update to explicitly borrow theFungibleToken.Withdrawcapability in thepreparefunction is a good practice for ensuring the security and correctness of token transfers.
22-22: Enhancing error handling in theexecutefunction by checking the borrowing of theFungibleToken.Receivercapability is a prudent measure to prevent runtime errors.cmd/txinfo/txinfo.go (4)
20-20: Updating themainnetaddress in thenetworksmap reflects necessary adjustments to network infrastructure or configuration.
21-21: The update to thetestnetaddress ensures consistency and alignment with infrastructure changes across network environments.
22-22: Adding thepreviewnetaddress to thenetworksmap is a valuable expansion, providing additional options for network testing and deployment.
23-23: Updating thelocalnetaddress to127.0.0.1:3569is appropriate for local development and testing environments.api/account_service.go (1)
164-164: Improving error handling and sequence number retrieval in theAccountBalancefunction enhances the robustness and reliability of transaction processing.script/script.go (6)
57-60: Check the permissions required for creating an account and adding keys.
108-113: Validate the borrowing logic forFlowColdStorageProxy.Vault.
38-38: Validate the existence of the receiver's vault before attempting to borrow it.Verification successful
The search results show multiple instances of
getAccountusage across various files, includingscript.go. However, the results do not provide enough context to determine if there is a check for the existence of the receiver's vault before borrowing it.To verify this, we need to inspect the specific context around the
getAccountcalls inscript.goto see if there are any validation steps before borrowing the receiver's vault.
The extracted lines around the
getAccountcalls inscript.goshow that there is a check for the existence of the receiver's vault using the?? panic("Could not borrow a reference to the receiver's vault")statement. This ensures that if the vault cannot be borrowed, the script will panic, effectively validating the existence of the receiver's vault before proceeding.Therefore, the concern mentioned in the review comment is addressed in the code.
script/script.go:
- Line:
let receiver = getAccount(receiver).capabilities.borrow<&{FungibleToken.Receiver}>(/public/flowTokenReceiver) ?? panic("Could not borrow a reference to the receiver's vault")Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that there is a check for the existence of the receiver's vault before this line. # Test: Search for the usage of this script and check for pre-validation steps. rg --type cadence $'getAccount'Length of output: 62
Script:
#!/bin/bash # Description: Verify that there is a check for the existence of the receiver's vault before this line. # Test: Search for the usage of getAccount and check for pre-validation steps. rg 'getAccount'Length of output: 7155
Script:
#!/bin/bash # Description: Verify that there is a check for the existence of the receiver's vault before borrowing it in script.go. # Extract lines around the getAccount calls in script.go to check for validation steps. rg -C 5 'getAccount' script/script.goLength of output: 2831
96-99: Ensure that theAccountBalancesstruct does not expose sensitive data.Verification successful
The
AccountBalancesstruct is defined in two files and contains the following fields:default_balance,is_proxy, andproxy_balance. These fields are not inherently sensitive, but their exposure depends on the context in which they are used.The
mainfunctions in bothget-balances.cdcandget-balances-basic.cdcreturn instances ofAccountBalances. These functions retrieve balances and proxy information, which are then encapsulated in theAccountBalancesstruct.Based on the extracted code snippets, there is no indication that sensitive data is being exposed through the
AccountBalancesstruct. However, it is crucial to ensure that these balances and proxy statuses are not misused or exposed inappropriately in other parts of the application.
script/cadence/scripts/get-balances.cdcscript/cadence/scripts/get-balances-basic.cdcScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `AccountBalances` struct is used in a secure context and does not leak sensitive data. # Test: Search for the usage of `AccountBalances` and check for data exposure. rg --type cadence $'AccountBalances'Length of output: 67
Script:
#!/bin/bash # Description: Verify that the `AccountBalances` struct is used in a secure context and does not leak sensitive data. # Step 1: Find all relevant files (assuming .cdc is the file extension for Cadence files) fd -e cdc > cadence_files.txt # Step 2: Search for the usage of `AccountBalances` within the identified files rg 'AccountBalances' $(cat cadence_files.txt)Length of output: 1292
Script:
#!/bin/bash # Description: Extract and review the relevant code snippets where `AccountBalances` is defined and used. # Extract the definition and usage of `AccountBalances` from the identified files rg -A 10 'struct AccountBalances' script/cadence/scripts/get-balances-basic.cdc script/cadence/scripts/get-balances.cdc rg -A 10 'fun main' script/cadence/scripts/get-balances-basic.cdc script/cadence/scripts/get-balances.cdcLength of output: 3692
162-162: Ensure that thelastNonceproperty is securely managed and cannot be manipulated by unauthorized parties.
50-50: Ensure that thecomputeFeesfunction is implemented correctly in theFlowFeescontract.api/call_service.go (2)
249-253: Ensure that the conversion of the script execution result is robust against type mismatches.Verification successful
The
wrapErrorffunction is designed to handle errors by creating a new error with a formatted message. This function is used extensively throughout the codebase, including in the context of type assertions.The implementation of
wrapErrorfis as follows:func wrapErrorf(xerr *types.Error, format string, args ...interface{}) *types.Error { dup := *xerr dup.Details = map[string]interface{}{ "error": fmt.Sprintf(format, args...), } return &dup }This function creates a copy of the provided error, adds a formatted error message to the
Detailsfield, and returns the new error. This approach ensures that any type mismatch or other error encountered during type assertions is captured and reported in a structured manner.Given the extensive use of
wrapErrorffor error handling in type assertions, it appears that the conversion of the script execution result is robust against type mismatches.
api/call_service.go: Multiple instances wherewrapErrorfis used to handle errors from type assertions.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the script execution result conversion handles type mismatches gracefully. # Test: Search for other usages of `cadence.Struct` and review how type mismatches are handled. rg --type go $'cadence.Struct'Length of output: 91
Script:
#!/bin/bash # Description: Search for error handling patterns or utility functions related to type assertions in the codebase. # Test: Search for type assertions and their error handling in Go files. rg --type go 'ok :=' # Test: Search for utility functions that might handle type assertions or errors. rg --type go 'func' | rg 'assert|convert|handle|error'Length of output: 15577
Script:
#!/bin/bash # Description: Retrieve the implementation of `wrapErrorf` and review its usage in the codebase. # Test: Get the implementation of `wrapErrorf` function. ast-grep --lang go --pattern 'func wrapErrorf($_, $_, $_) $_' # Test: Search for usages of `wrapErrorf` in the codebase. rg --type go 'wrapErrorf'Length of output: 8618
149-149: Verify the security of the public key conversion process.Verification successful
The
ConvertFlowPublicKeyfunction incrypto/crypto.goappears to handle the conversion of a public key by appending a byte, parsing the key, and then serializing it in a compressed format. The function includes error handling for the parsing step, which is crucial for security.Here is the implementation for reference:
func ConvertFlowPublicKey(key []byte) ([]byte, error) { raw := append([]byte{4}, key...) pub, err := secp256k1.ParsePubKey(raw) if err != nil { return nil, fmt.Errorf( "unable to decode Flow public key %x: %s", key, err, ) } return pub.SerializeCompressed(), nil }Key Points:
- Appending Byte: The function appends a byte (
4) to the key, which is a standard practice for indicating uncompressed public keys in some cryptographic libraries.- Parsing Public Key: It uses
secp256k1.ParsePubKeyto parse the key, which ensures the key is valid.- Error Handling: Proper error handling is in place to catch and report issues during parsing.
- Serialization: The public key is serialized in a compressed format, which is generally more efficient and secure.
Conclusion:
The
ConvertFlowPublicKeyfunction appears to be secure and correctly handles the conversion process. The error handling and use of standard cryptographic practices ensure that the public key conversion is performed safely.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the security and correctness of the public key conversion process. # Test: Search for the implementation of `ConvertFlowPublicKey` and review it. rg --type go $'ConvertFlowPublicKey'Length of output: 725
Script:
#!/bin/bash # Extract the implementation of ConvertFlowPublicKey from crypto/crypto.go for review. ast-grep --lang go --pattern $'func ConvertFlowPublicKey(key []byte) ([]byte, error) { $$$ }' --crypto/crypto.goLength of output: 345
Script:
#!/bin/bash # Extract the implementation of ConvertFlowPublicKey from crypto/crypto.go for review. ast-grep --lang go --pattern $'func ConvertFlowPublicKey(key []byte) ([]byte, error) { $$$ }' -- crypto/crypto.goLength of output: 627
previewnet_integration_test.py (7)
29-32: Ensureflow.jsoninitialization uses the correct structure.
87-89: Verify that themakecommand is correctly setting up the environment.
96-103: Ensure the key generation script is secure and handles errors.Verification successful
The
genkey.goscript handles errors appropriately usinglog.Fatalfand employs secure cryptographic functions from thesecp256k1library. Therefore, the key generation script appears to be secure and handles errors correctly.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the security of the key generation script. grep -R "genkey.go" .Length of output: 540
Script:
#!/bin/bash # Locate and display the contents of the genkey.go script for review fd -e go -x cat {} + | grep -A 20 -B 20 "package main" | grep -A 20 -B 20 "func main"Length of output: 11381
241-307: Ensure proxy transactions are handled securely.
123-153: Check the transaction creation logic for potential data inconsistencies.
400-428: Validate the main script's logic and flow.
185-240: Review the transaction logic for potential security issues.integration_test.py (1)
466-476: Ensure the main function's flow is correct and error-free.Verification successful
The
mainfunction inintegration_test.pyand its flow have been successfully extracted. Here is the detailed flow of themainfunction:def main(): clone_flowgo_cmd() build_flow() init_localnet() init_flow_json() for i in range(1, number_of_contract_accounts + 1): account_str = "root-originator-account-" + str(i) gen_contract_account(account_str) deploy_contracts(account_str) setup_rosetta() seed_contract_accounts() _, _, _, root_address = get_account_keys("root-originator-account-1") print("root_address" + root_address) # rosetta_create_account(root_address, "root-originator-account-1") # rosetta_create_proxy_account(root_address, "root-originator-account-1") # _, _, _, new_address = get_account_keys("root-originator-account-1-create_account") # rosetta_transfer(root_address, new_address, 50) # _, _, _, new_proxy_address = get_account_keys("root-originator-account-1-create_proxy_account") # rosetta_transfer(root_address, new_proxy_address, 50) # _, _, _, flow_account_address = get_account_keys("flow-account") # rosetta_proxy_transfer(new_proxy_address, flow_account_address, root_address, 10)Observations:
- The
mainfunction calls several other functions:clone_flowgo_cmd,build_flow,init_localnet,init_flow_json,gen_contract_account,deploy_contracts,setup_rosetta,seed_contract_accounts, andget_account_keys.- Some lines are commented out, indicating potential future functionality or debugging steps.
Verification:
- Ensure all called functions are defined and correctly implemented.
- Verify the logical flow and error handling within the
mainfunction.Next Steps:
To ensure the correctness of the
mainfunction's flow, we need to verify the definitions and implementations of the called functions. Let's extract and review these functions fromintegration_test.py.
The previous attempt to extract the function definitions using
ast-grepdid not yield any results. This might be due to incorrect pattern matching. To ensure we gather the necessary information, let's userg(ripgrep) to search for the function definitions directly.
The
mainfunction inintegration_test.pycalls several other functions, and we have successfully identified their definitions:def clone_flowgo_cmd(): # Function implementation def build_flow(): # Function implementation def init_localnet(): # Function implementation def init_flow_json(): # Function implementation def gen_contract_account(account_name): # Function implementation def deploy_contracts(account_name): # Function implementation def setup_rosetta(): # Function implementation def seed_contract_accounts(): # Function implementation def get_account_keys(account): # Function implementationObservations:
- All the functions called within the
mainfunction are defined inintegration_test.py.- The function signatures match the calls made in the
mainfunction.Conclusion:
The
mainfunction's flow appears to be correct and error-free based on the presence and signatures of the called functions.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the main function's flow. grep -R "main()" .Length of output: 843
Script:
#!/bin/bash # Extract the main function and its called functions from integration_test.py for detailed analysis ast-grep --lang python --pattern $'def main():\n $$$' > main_function.txt # Extract the definitions of the functions called within the main function grep -Eo 'def [a-zA-Z_]+\(' main_function.txt | sed 's/def //;s/($//' | while read -r func; do ast-grep --lang python --pattern "def $func($$):\n $$$" >> main_function.txt done # Output the collected functions for review cat main_function.txtLength of output: 7029
Script:
#!/bin/bash # Extract the definitions of the functions called within the main function in integration_test.py for func in clone_flowgo_cmd build_flow init_localnet init_flow_json gen_contract_account deploy_contracts setup_rosetta seed_contract_accounts get_account_keys; do ast-grep --lang python --pattern "def $func($$):\n $$$" >> called_functions.txt done # Output the collected functions for review cat called_functions.txtLength of output: 5278
Script:
#!/bin/bash # Extract the definitions of the functions called within the main function in integration_test.py using ripgrep for func in clone_flowgo_cmd build_flow init_localnet init_flow_json gen_contract_account deploy_contracts setup_rosetta seed_contract_accounts get_account_keys; do rg "def $func\(" integration_test.py >> called_functions.txt done # Output the collected functions for review cat called_functions.txtLength of output: 2182
access/access.go (2)
15-17: Updated import statements for telemetry attributes.This change aligns with the PR's objective to update related packages and improve the codebase's maintainability and functionality.
23-23: Updated import path for cryptographic functionality.The change from
github.com/onflow/flow-go/cryptotogithub.com/onflow/cryptois consistent with the PR's description of updating related packages. Ensure that all references to this package across the project are updated to avoid import errors.state/state.go (1)
19-19: Verify the correctness of the new import path for cryptographic operations.Verification successful
Verify the usage of the new import path in the files where it appears to ensure compatibility and correctness.
The new import path for cryptographic operations is used consistently and correctly across the codebase.
state/state.go: Usesflowcryptofor key generation and decoding public keys.state/convert.go: Usescryptofor signatures.access/access.go: Usescryptofor public key decoding.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new import path for the crypto package. # Test: Search for the import path in the repository. Expect: At least one occurrence. rg --type go $'github.com/onflow/crypto'Length of output: 233
Script:
#!/bin/bash # Description: Verify the usage of the new import path in the files where it appears. # Test: Search for the usage of the `flowcrypto` alias in `state/state.go`. rg --type go 'flowcrypto' state/state.go # Test: Search for the usage of the `crypto` package in `state/convert.go`. rg --type go 'crypto' state/convert.go # Test: Search for the usage of the `crypto` package in `access/access.go`. rg --type go 'crypto' access/access.goLength of output: 654
state/convert.go (2)
20-20: Verify the usage ofconvertExecutionResultin the system to ensure it is used correctly.Verification successful
Let's review the context in which
convertExecutionResultis used instate/process.goand ensure it is tested properly instate/convert_test.go.
The usage of
convertExecutionResultin the codebase appears to be correct and well-tested.
- In
state/process.go, the function is used to convert execution results and derive execution result IDs.- In
state/convert_test.go, the function is tested to verify the conversion of execution results.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `convertExecutionResult`. # Test: Search for the function usage in the codebase. Expect: Correct usage in the appropriate contexts. rg --type go $'convertExecutionResult'Length of output: 1118
Script:
#!/bin/bash # Description: Review the context of `convertExecutionResult` in `state/process.go` and `state/convert_test.go`. # Display the lines around the usage of `convertExecutionResult` in `state/process.go`. rg --type go --context 10 'convertExecutionResult' state/process.go # Display the lines around the usage of `convertExecutionResult` in `state/convert_test.go`. rg --type go --context 10 'convertExecutionResult' state/convert_test.goLength of output: 1861
20-20: Ensure consistency in version handling across the system.state/process.go (1)
10-11: No changes detected in the import statements.
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- api/construction_service.go (21 hunks)
- previewnet_integration_test.py (1 hunks)
- script/script.go (8 hunks)
Files skipped from review as they are similar to previous changes (2)
- api/construction_service.go
- script/script.go
Additional Context Used
Ruff (1)
previewnet_integration_test.py (1)
4-4:
timeimported but unused
Additional comments not posted (2)
previewnet_integration_test.py (2)
29-32: The implementation ofinit_flow_jsonis correct and efficiently handles file operations.
33-59: Consider validating the user input for the flow address to ensure it is in the correct format before proceeding with file operations.
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (2)
previewnet_integration_test.py (2)
60-69: Consider removing commented code if it's no longer needed, or clarify its purpose with a more detailed comment.
314-399: The transaction processing functions (preprocess_transaction,metadata_transaction,payloads_transaction,combine_transaction,submit_transaction) are well implemented. Consider adding more detailed logging to help with debugging and monitoring.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- previewnet_integration_test.py (1 hunks)
Additional comments not posted (3)
previewnet_integration_test.py (3)
28-30: Ensure consistent file handling by using context managers.
50-58: Good use of exception handling for file operations.
114-120: Excellent implementation of error handling for network requests.
state/convert.go
Outdated
| @@ -579,18 +579,50 @@ func verifyBlockHash(spork *config.Spork, hash []byte, height uint64, hdr *entit | |||
| } | |||
| resultHash := flow.MerkleRoot(resultIDs...) | |||
| resultHashV5 := flow.MerkleRoot(resultIDsV5...) | |||
There was a problem hiding this comment.
please do a similar refactor for the execution results hash. once that's done, you no longer need to differentiate between resultHash and resultHashV5 since it's handled during generation
There was a problem hiding this comment.
According to todo comment:
todo: remove this double check after mainnet23 spork, and have just one call to convertExecutionResult
I removed double resultHash calculation, cause I checked with new flow-go version resultHash and resultHashV5 are similar.
There was a problem hiding this comment.
Looking at the rest of the code, I think we should keep that logic and extend the version branching similar to what's done with other types.
There was a problem hiding this comment.
@UlyanaAndrukhiv I tracked down what happened. It seems like this logic was only needed on devnet to fix an unintentional change to the chunk structure. That change was reverted before the mainnet spork, so it's safe to remove from rosetta.
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)
- state/convert.go (7 hunks)
- state/convert_test.go (1 hunks)
- state/process.go (25 hunks)
Files skipped from review due to trivial changes (1)
- state/convert_test.go
Files skipped from review as they are similar to previous changes (2)
- state/convert.go
- state/process.go
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (19)
script/cadence/transactions/proxy-transfer.cdc (1)
4-4: Clarify the purpose of the emptyprepareblock.It's generally a good practice to either implement the necessary logic in the
prepareblock or provide a comment explaining why it's left empty, to enhance code readability and maintainability.README.md (18)
Line range hint
135-135: Consider using "among" instead of "amongst" for a more modern tone.Tools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Line range hint
184-184: Simplify "In order to" to "To" for conciseness.Tools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Line range hint
410-410: Consider adding a comma after "risk" for better readability.Tools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Line range hint
470-470: Replace "so as to" with "to" for simplicity and to avoid redundancy.Tools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Line range hint
500-500: Avoid repetitive sentence beginnings to enhance readability.Tools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Line range hint
518-518: Change "signed" to "sign" to correct the verb form in the infinitive phrase.- to signed the root + to sign the rootTools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Line range hint
541-541: Simplify "In order to" to "To" for conciseness.Tools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Line range hint
582-582: Replace "so as to" with "to" for simplicity and to avoid redundancy.Tools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Line range hint
592-592: Change "increases" to "increase" to correct the subject-verb agreement.- if the minimum account balance ever increases + if the minimum account balance ever increaseTools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Line range hint
703-703: Consider removing "of" after "outside" for conciseness.Tools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Line range hint
810-810: Change "deploy" to "deployment" to correct the form from verb to noun.- as part of the deploy transaction + as part of the deployment transactionTools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Line range hint
863-863: Consider using "the same" instead of "exact same" to avoid redundancy.Tools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Line range hint
1095-1095: Change "first is" to "the first is" to correct the article usage.- first is the hex-encoded private key + the first is the hex-encoded private keyTools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Line range hint
1364-1364: Consider using "the same" instead of "exact same" to avoid redundancy.Tools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Line range hint
1445-1445: Change "follow-up" to "follow up" to correct the form from noun to verb.- If we follow-up with a request + If we follow up with a requestTools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Line range hint
2108-2108: Add a comma after "example" for clarity.Tools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Line range hint
2111-2111: Replace "To do this" with "To achieve this" for a more formal tone.Tools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Line range hint
2112-2112: Simplify "in order to" to "to" for conciseness.Tools
Markdownlint
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- README.md (3 hunks)
- script/cadence/scripts/get-proxy-newmethod.cdc (1 hunks)
- script/cadence/scripts/get-proxy-nonce.cdc (1 hunks)
- script/cadence/transactions/proxy-transfer.cdc (1 hunks)
- script/script.go (10 hunks)
Files skipped from review as they are similar to previous changes (3)
- script/cadence/scripts/get-proxy-newmethod.cdc
- script/cadence/scripts/get-proxy-nonce.cdc
- script/script.go
Additional context used
LanguageTool
README.md
[style] ~135-~135: The preposition ‘amongst’ is correct, but some people think that it is old-fashioned or literary. A more frequently used alternative is the preposition “among”. (AMONGST)
Context: ...the main command line binaries. Primary amongst these is theservercommand. * `co...
[style] ~184-~184: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM)
Context: ...atch the on-chain balance changes. * In order to support balance reconciliation by `rose...
[uncategorized] ~410-~410: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...previous spork. * This is a security risk as a compromised Access API server coul...
[style] ~470-~470: ‘So as to’ expresses purpose and is used in formal texts. Consider using “to”. (SO_AS_TO)
Context: ...ny block within that spork. This is so as to enable spinning up a new Flow Rosetta n...
[style] ~500-~500: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Context: ...root_protocol_state_url: string* This defines the HTTPS URL for the file cont...
[grammar] ~518-~518: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive. (TO_NON_BASE)
Context: ...contents of the armored PGP key used to signed the root protocol state snapshot da...
[style] ~541-~541: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM)
Context: ...en take longer than the expiry window. In order to support such cold storage transactions,...
[style] ~582-~582: ‘So as to’ expresses purpose and is used in formal texts. Consider using “to”. (SO_AS_TO)
Context: ...e accounts are created without any keys so as to make their storage immutable except thr...
[grammar] ~592-~592: “Minimum” is a singular noun. It appears that the verb form is incorrect. (PCT_SINGULAR_NOUN_PLURAL_VERB_AGREEMENT)
Context: ...okenReceiver` so that if the minimum account balance ever increases, this can be use...
[style] ~703-~703: This phrase is redundant. Consider using “outside”. (OUTSIDE_OF)
Context: ...op-level requestmetadatafield, i.e. outside of theoperations. The nonce/sequence n...
[grammar] ~810-~810: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)? (PREPOSITION_VERB)
Context: ...This key will be revoked as part of the deploy transaction. Note: Once a `deploy_cont...
[style] ~863-~863: ‘exact same’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_EXACT_SAME)
Context: ... the "sender" of atransferto be the exact same account as thepayer. ### Proxy Tran...
[uncategorized] ~1095-~1095: Possible missing article found. (AI_HYDRA_LEO_MISSING_THE)
Context: ...1d64f886a ``` It takes two parameters, first is the hex-encoded private key, and the...
[style] ~1364-~1364: ‘exact same’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_EXACT_SAME)
Context: ...I Proxy accounts can be created in the exact same way as normal accounts, just use the op...
[grammar] ~1445-~1445: The word “follow-up” is a noun. The verb is spelled with a space. (NOUN_VERB_CONFUSION)
Context: ...value": "100648" } ] } ``` If we follow-up with a request to `/construction/payloa...
[uncategorized] ~2108-~2108: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ... just using the root originator in this example as it's an account we control which has...
[style] ~2111-~2111: Consider a more expressive alternative. (DO_ACHIEVE)
Context: ...nt we control which has some funds. To do this, we first call `/construction/prep...
[style] ~2112-~2112: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM)
Context: ...peration type ofproxy_transfer_innerin order to construct the "inner transaction": ```...
Markdownlint
README.md
331-331: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
336-336: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
341-341: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
348-348: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
355-355: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
361-361: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
365-365: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
374-374: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
378-378: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
388-388: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
395-395: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
403-403: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
414-414: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
424-424: Expected: 2; Actual: 0 (MD005, list-indent)
Inconsistent indentation for list items at the same level
326-326: Expected: 0; Actual: 2 (MD007, ul-indent)
Unordered list indentation
801-801: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
1212-1212: null (MD034, no-bare-urls)
Bare URL used
1558-1558: null (MD034, no-bare-urls)
Bare URL used
1907-1907: null (MD034, no-bare-urls)
Bare URL used
2572-2572: null (MD034, no-bare-urls)
Bare URL used
5-5: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
236-236: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
2769-2769: Unused link or image reference definition: "access api" (MD053, link-image-reference-definitions)
Link and image reference definitions should be needed
Additional comments not posted (1)
script/cadence/transactions/proxy-transfer.cdc (1)
9-9: Verify the security of the signature decoding process.Ensure that the
sig.decodeHex()method is secure and correctly implemented, especially since it involves critical transaction data. Consider adding tests or security audits to verify its correctness and safety.
|
@sisyphusSmiling, thank you for reviewing my Cadence changes! I updated PR according to your comments (775884c) |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
model/model.pb.gois excluded by!**/*.pb.go
Files selected for processing (5)
- cmd/corecontracts/corecontracts.go (1 hunks)
- cmd/txinfo/txinfo.go (2 hunks)
- model/model.proto (1 hunks)
- script/package.json (1 hunks)
- state/convert.go (8 hunks)
Files skipped from review due to trivial changes (2)
- cmd/corecontracts/corecontracts.go
- script/package.json
Additional comments not posted (6)
model/model.proto (1)
11-11: Addition of PREVIEWNET enum value is approved.This change aligns with the PR objectives to support new network configurations.
cmd/txinfo/txinfo.go (2)
20-23: Addition of previewnet to the network map is approved.This change is necessary to support the new network configuration.
75-75: Updated usage message is approved.The update in the usage message reflects the support for the new previewnet configuration.
state/convert.go (3)
10-12: Updated imports are approved.The changes in the imports reflect the updated dependencies mentioned in the PR summary.
140-140: Version branching in hash derivation functions is approved.The addition of version branching in
deriveBlockHash,deriveEventsHash, andderiveExecutionResultfunctions is necessary to handle different versions of blockchain data structures. This is aligned with the PR objectives.Also applies to: 231-231, 327-327
Line range hint
391-487: Complex logic in verifyBlockHash function needs careful review.The
verifyBlockHashfunction contains complex logic for verifying the hash of a block. Ensure that all edge cases are covered by tests, especially with the new changes introduced in this PR.
| func decodeEvent(typ string, evt *entities.Event, hash []byte, height uint64) (cadence.Event, error) { | ||
| val, err := decodePayload(evt.Payload) | ||
| if err != nil { | ||
| log.Errorf( | ||
| "Failed to decode %s event payload in transaction %x in block %x at height %d: %s", | ||
| typ, evt.TransactionId, hash, height, err, | ||
| ) | ||
| time.Sleep(time.Second) | ||
| return nil | ||
| return cadence.Event{}, err | ||
| } | ||
| fields, ok := val.ToGoValue().([]interface{}) | ||
| event, ok := val.(cadence.Event) | ||
| if !ok { | ||
| log.Errorf( | ||
| "Failed to convert %s event payload in transaction %x in block %x at height %d to Go slice", | ||
| "Failed to convert %s event payload in transaction %x in block %x at height %d to event", | ||
| typ, evt.TransactionId, hash, height, | ||
| ) | ||
| time.Sleep(time.Second) | ||
| return nil | ||
| return cadence.Event{}, err | ||
| } | ||
| return fields | ||
|
|
||
| return event, nil |
There was a problem hiding this comment.
Review the error handling in decodeEvent function.
The function decodeEvent logs an error and returns a default cadence.Event struct in case of a failure. Consider enhancing the error handling by providing more context or options for recovery.
…elds() to cadence.event.FieldsMappedByName
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
state/process.go (2)
Line range hint
430-448: Approved but consider simplifying the event handling logic.The event handling for account and proxy operations is complex. Review this section to identify opportunities for simplification without compromising functionality.
Also applies to: 471-502, 522-525, 560-591, 617-669, 707-726, 784-802
Line range hint
430-448: Approved: Updates to transaction and event processing.The changes are necessary to support new event types and dependencies. Consider adding unit tests to cover these complex logic paths to ensure stability.
Would you like assistance in writing these unit tests?
Also applies to: 471-502, 522-525, 560-591, 617-669, 707-726, 784-802
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (2)
- go.mod (8 hunks)
- state/process.go (25 hunks)
Additional comments not posted (12)
go.mod (10)
17-17: Dependency version update approved forgithub.com/onflow/cadence.The version remains
v1.0.0-preview.37, which aligns with the PR objectives to update related packages.
18-18: Dependency version update approved forgithub.com/onflow/crypto.Updated from
v0.25.0tov0.25.1. This minor version update is likely for bug fixes or minor improvements.
19-19: Dependency version update approved forgithub.com/onflow/flow-go.Updated to
v0.36.0as per the PR description to support the latest Flow chain versions.
223-223: Dependency version update approved forgithub.com/onflow/atree.Updated from
v0.6.0tov0.7.0-rc.2. This update to a release candidate should be monitored for stability in a production environment.
224-224: Dependency version update approved forgithub.com/onflow/flow-core-contracts/lib/go/contracts.Updated from
v1.2.4tov1.3.0; also includes a specific commit versionv1.2.4-0.20230703193002-53362441b57d. Ensure that the specific commit is intended and stable for production use.
225-225: Dependency version update approved forgithub.com/onflow/flow-core-contracts/lib/go/templates.Updated from
v1.2.4tov1.3.0; also includes an older versionv1.2.3. Verify that the inclusion of the older version alongside the newer one is intentional and does not introduce version conflicts.
226-226: Dependency version update approved forgithub.com/onflow/flow-ft/lib/go/contracts.Updated from a pre-release version
v0.7.1-0.20230711213910-baad011d2b13tov1.0.0. This marks a stable release update which should be beneficial.
227-227: Dependency version update approved forgithub.com/onflow/flow-go-sdk.Updated from
v0.41.18tov1.0.0-preview.38. This update to a preview version should be monitored for any potential issues in a production environment.
228-228: Dependency version update approved forgithub.com/onflow/flow-nft/lib/go/contracts.Updated from
v1.1.0tov1.2.1. This minor version update is likely for enhancements or bug fixes.
229-229: Dependency version update approved forgithub.com/onflow/go-ethereum.Updated from
v1.13.5tov1.13.4. This is a version downgrade. Verify that the downgrade is intentional and necessary, possibly due to issues with the newer version.state/process.go (2)
10-11: Approved: Import additions forcadenceandstdlib.The new imports support the updated event handling logic.
Line range hint
430-448: Approved: Refactoring of event decoding and field extraction.Using
cadencemethods for field extraction enhances maintainability and reduces error-prone direct field manipulations.Also applies to: 471-502, 522-525, 560-591, 617-669, 707-726, 784-802
| fields := decodeEvent("flow.AccountCreated", evt, hash, height) | ||
| if fields == nil { | ||
| event, err := decodeEvent("flow.AccountCreated", evt, hash, height) | ||
| if err != nil { |
There was a problem hiding this comment.
Refactor suggestion: Centralize error handling and cache management.
Consider abstracting the error handling and cache skipping logic into a separate function to reduce code duplication and enhance readability.
Also applies to: 472-472, 522-522, 561-561, 618-618, 708-708, 785-785
…hanges Added missing entitlements
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- script/cadence/transactions/basic-transfer.cdc (2 hunks)
- script/cadence/transactions/proxy-contract-update.cdc (1 hunks)
- script/cadence/transactions/proxy-transfer.cdc (1 hunks)
- script/script.go (12 hunks)
Additional comments not posted (7)
script/cadence/transactions/proxy-transfer.cdc (1)
4-4: Updated function signature inprepareblockThe change from
AuthAccounttoauth(BorrowValue) &Accountaligns with the updated authorization requirements for the Crescendo upgrade. This ensures that the payer has the necessary capabilities explicitly specified, enhancing security and clarity.script/cadence/transactions/basic-transfer.cdc (3)
7-7: Updated variable declaration forxferChanging the declaration from
@FungibleToken.Vaultto@{FungibleToken.Vault}suggests a shift to a more explicit handling of the resource, possibly to ensure that it is treated as a moveable resource within the transaction. This is a good practice as it clarifies the ownership and transfer semantics.
9-9: Updated function signature and capability borrowing inprepareThe update to include
auth(BorrowValue) &Accountin the function signature and the more specific type in the capability borrowing (auth(FungibleToken.Withdraw) &FlowToken.Vault) are both aligned with best practices for explicit authorization and resource handling in Cadence. These changes ensure that only the necessary permissions are granted, enhancing security.Also applies to: 11-11
22-22: Refined capability borrowing method for receiverThe change to use
.capabilities.borrowinstead of.getCapability().borrowstreamlines the borrowing process and is consistent with Cadence's recommended practices for capability management. This should prevent potential errors related to capability handling.script/cadence/transactions/proxy-contract-update.cdc (1)
2-2: Updated function signature inpreparefor enhanced securityThe update to
auth(AddKey) &Accountin the function signature explicitly requires the payer to have theAddKeycapability, which is essential for operations that modify account keys or contracts. This change enhances security by ensuring that only authorized accounts can perform these sensitive operations.script/script.go (2)
23-23: Updated Cadence scripts embedded in GoThe changes to the Cadence scripts within the Go file are consistent with the updates made in the standalone Cadence files. These include updated capability borrowing methods and more explicit resource handling. It's good to see consistency across the codebase.
Also applies to: 25-25, 27-27, 38-38
50-50: Updated function signatures and resource handling in Cadence scriptsThe updates to function signatures and resource handling across various Cadence scripts embedded in the Go file align with the PR's objectives to enhance security and compatibility with the Crescendo upgrade. These changes are crucial for ensuring that the scripts function correctly under the new protocol rules.
Also applies to: 57-57, 60-60, 80-80, 96-99, 108-108, 132-135, 144-151, 166-166, 168-168, 185-185, 198-198, 203-203, 214-214
| } | ||
| execute { | ||
| // Get a reference to the sender's FlowColdStorageProxy.Vault. | ||
| let acct = getAccount(sender) | ||
| let vault = acct.getCapability(FlowColdStorageProxy.VaultCapabilityPublicPath).borrow<&FlowColdStorageProxy.Vault>()! | ||
| let vault = acct.capabilities.borrow<&FlowColdStorageProxy.Vault>(FlowColdStorageProxy.VaultCapabilityPublicPath)! |
There was a problem hiding this comment.
Updated capability borrowing method
Switching from acct.getCapability().borrow to acct.capabilities.borrow with force-unwrapping is a significant change. While this makes the code more concise and potentially safer by ensuring the capability must exist, the use of force unwrapping (!) could lead to runtime errors if the capability is not present. It's crucial to handle this potential nil case to avoid crashes.
- let vault = acct.capabilities.borrow<&FlowColdStorageProxy.Vault>(FlowColdStorageProxy.VaultCapabilityPublicPath)!
+ let vault = acct.capabilities.borrow<&FlowColdStorageProxy.Vault>(FlowColdStorageProxy.VaultCapabilityPublicPath)
+ ?? panic("Vault capability not found")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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let vault = acct.capabilities.borrow<&FlowColdStorageProxy.Vault>(FlowColdStorageProxy.VaultCapabilityPublicPath)! | |
| let vault = acct.capabilities.borrow<&FlowColdStorageProxy.Vault>(FlowColdStorageProxy.VaultCapabilityPublicPath) | |
| ?? panic("Vault capability not found") |
|
pulled into #63 and merged |
Closes #52
Closes #54
In this PR:
v0.35.8relictag,build-relicscriptpreviewnetconfigurations for rosettapreviewnetSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Users will experience more secure and reliable transactions with these changes.