feat: implement on-chain contract version tracking for split-escrow#252
Open
fredericklamar342-prog wants to merge 2 commits intoOlufunbiIK:mainfrom
Open
feat: implement on-chain contract version tracking for split-escrow#252fredericklamar342-prog wants to merge 2 commits intoOlufunbiIK:mainfrom
fredericklamar342-prog wants to merge 2 commits intoOlufunbiIK:mainfrom
Conversation
|
Someone is attempting to deploy a commit to the olufunbiik's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds on-chain contract versioning: store a semver at initialization, expose a getter, allow admin-only upgrades with semver validation, emit a ContractUpgraded event, add an InvalidVersion error, and update tests and fee collection behavior. Changes
Sequence DiagramsequenceDiagram
participant Admin as Admin/Client
participant Contract as SplitEscrow Contract
participant Storage as Storage Layer
participant Events as Event System
Admin->>Contract: initialize(admin, token, version:"1.0.0")
Contract->>Contract: validate semver (is_valid_semver)
Contract->>Storage: set_version("1.0.0")
Storage-->>Contract: ok
Admin->>Contract: upgrade_version("1.1.0")
Contract->>Contract: verify admin auth
Contract->>Contract: validate semver("1.1.0")
Contract->>Storage: get_version() -> "1.0.0"
Storage-->>Contract: "1.0.0"
Contract->>Storage: set_version("1.1.0")
Storage-->>Contract: ok
Contract->>Events: emit_contract_upgraded("1.0.0","1.1.0")
Events-->>Admin: ContractUpgraded event
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
contracts/split-escrow/src/storage.rs (1)
84-86: Consider returningOption<String>for migration safety.The
unwrap()will panic if called on a contract that was initialized before this version tracking feature was added. Unlikeget_adminwhich is guaranteed to exist after initialization,Versionwon't exist for pre-upgrade contracts.Consider following the pattern used by
get_treasury(line 76-78) which returnsOption<Address>:♻️ Proposed change
-pub fn get_version(env: &Env) -> soroban_sdk::String { - env.storage().instance().get(&DataKey::Version).unwrap() +pub fn get_version(env: &Env) -> Option<soroban_sdk::String> { + env.storage().instance().get(&DataKey::Version) }Then callers can provide a default or handle the missing case gracefully.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/split-escrow/src/storage.rs` around lines 84 - 86, Change get_version to return Option<soroban_sdk::String> instead of unwrapping; read DataKey::Version from env.storage().instance().get(...) and return the Option so callers can handle missing versions (follow the get_treasury/get_admin pattern). Update any callers of get_version to handle None or provide a default version during migration/initialization flows.contracts/split-escrow/src/test.rs (1)
127-137: Unused variableadminin test.The variable
adminis captured fromsetup()but never used in this test.♻️ Proposed fix
#[test] fn test_upgrade_version_success() { - let (env, client, admin, _, _, _, _) = setup(); + let (env, client, _, _, _, _, _) = setup(); // Auth is mocked in setup, but we can explicitly test it if needed. // Here we just test the functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/split-escrow/src/test.rs` around lines 127 - 137, The test test_upgrade_version_success binds an unused admin variable from setup(); fix by dropping the unused binding—call setup() as let (env, client, _, _, _, _, _) = setup(); or rename admin to a prefixed underscore (e.g., _admin) so the compiler won’t warn; keep the rest of the test (client.upgrade_version and assert_eq!(client.get_version(), new_version)) unchanged.contracts/split-escrow/src/lib.rs (2)
143-158: Consider validating that the new version is greater than the old version.The current implementation allows "downgrades" (e.g., upgrading from
2.0.0to1.0.0). If version tracking is intended to reflect actual contract evolution, you may want to enforce monotonically increasing versions to prevent accidental or malicious rollbacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/split-escrow/src/lib.rs` around lines 143 - 158, The upgrade_version function currently only checks semver validity but allows downgrades; modify upgrade_version to retrieve the current version via storage::get_version(&env), parse both old and new using the existing is_valid_semver or a semver parser, compare them to ensure new_version > old_version (reject equal or lesser versions with Error::InvalidVersion or a new Error variant), and only call storage::set_version(&env, &new_version) and events::emit_contract_upgraded(&env, old_version, new_version) when the comparison passes; reference upgrade_version, is_valid_semver, storage::get_version, storage::set_version, and events::emit_contract_upgraded.
161-189: Leading zeros are not rejected, which violates semver spec.The current validation accepts versions like
"01.0.0"or"1.00.0", which are invalid per the semver specification (numeric identifiers must not include leading zeros).If strict semver compliance is needed, add a check to reject segments with leading zeros:
♻️ Proposed enhancement
if char == b'.' { if !segment_has_digit { return false; } dot_count += 1; segment_has_digit = false; + segment_start = true; } else if char >= b'0' && char <= b'9' { + // Reject leading zeros (except "0" alone, handled by next dot or end) + if segment_start && char == b'0' && i + 1 < len && buf[i + 1] != b'.' { + return false; + } + segment_start = false; segment_has_digit = true; } else { return false; }Alternatively, if lightweight validation is intentional for gas efficiency, document this deviation from strict semver.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/split-escrow/src/lib.rs` around lines 161 - 189, The is_valid_semver function currently accepts numeric identifiers with leading zeros; modify is_valid_semver to track each segment's start index/length (e.g., add segment_start or segment_len) while iterating over buf so that when you hit a '.' or the end you can reject the segment if segment_len > 1 and the first byte of that segment is b'0'; keep the existing checks (digit-only, three dots check) but add this leading-zero check before resetting segment_len/segment_has_digit and returning the final result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/split-escrow/src/test.rs`:
- Around line 158-168: The test test_semver_validation_at_init is missing a
mocked auth context so initialize (which calls admin.require_auth()) will fail
before semver checks; fix by calling env.mock_all_auths() at the start of the
test before invoking SplitEscrowContractClient::initialize (or before any
admin.require_auth() path) so the auth check is satisfied and the semver
validation/assertion can run.
- Around line 147-156: The test test_upgrade_version_non_admin is invalid
because it runs env.as_contract(&client.address, ...) (executing as the
contract) and setup() calls mock_all_auths(), and the declared non_admin is
never used; change the test to invoke client.upgrade_version as the non_admin
address (not as the contract) and ensure you do not mock all auths for this
call—either remove or restrict mock_all_auths() in setup() or reconfigure
mock_auths to exclude the non_admin principal, then call the contract using the
non_admin principal (instead of env.as_contract) so the require_auth check in
client.upgrade_version actually fails for non-admin.
---
Nitpick comments:
In `@contracts/split-escrow/src/lib.rs`:
- Around line 143-158: The upgrade_version function currently only checks semver
validity but allows downgrades; modify upgrade_version to retrieve the current
version via storage::get_version(&env), parse both old and new using the
existing is_valid_semver or a semver parser, compare them to ensure new_version
> old_version (reject equal or lesser versions with Error::InvalidVersion or a
new Error variant), and only call storage::set_version(&env, &new_version) and
events::emit_contract_upgraded(&env, old_version, new_version) when the
comparison passes; reference upgrade_version, is_valid_semver,
storage::get_version, storage::set_version, and events::emit_contract_upgraded.
- Around line 161-189: The is_valid_semver function currently accepts numeric
identifiers with leading zeros; modify is_valid_semver to track each segment's
start index/length (e.g., add segment_start or segment_len) while iterating over
buf so that when you hit a '.' or the end you can reject the segment if
segment_len > 1 and the first byte of that segment is b'0'; keep the existing
checks (digit-only, three dots check) but add this leading-zero check before
resetting segment_len/segment_has_digit and returning the final result.
In `@contracts/split-escrow/src/storage.rs`:
- Around line 84-86: Change get_version to return Option<soroban_sdk::String>
instead of unwrapping; read DataKey::Version from
env.storage().instance().get(...) and return the Option so callers can handle
missing versions (follow the get_treasury/get_admin pattern). Update any callers
of get_version to handle None or provide a default version during
migration/initialization flows.
In `@contracts/split-escrow/src/test.rs`:
- Around line 127-137: The test test_upgrade_version_success binds an unused
admin variable from setup(); fix by dropping the unused binding—call setup() as
let (env, client, _, _, _, _, _) = setup(); or rename admin to a prefixed
underscore (e.g., _admin) so the compiler won’t warn; keep the rest of the test
(client.upgrade_version and assert_eq!(client.get_version(), new_version))
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2dd95170-1472-4bc5-adfc-9f7eb0f2279e
📒 Files selected for processing (5)
contracts/split-escrow/src/errors.rscontracts/split-escrow/src/events.rscontracts/split-escrow/src/lib.rscontracts/split-escrow/src/storage.rscontracts/split-escrow/src/test.rs
Comment on lines
+158
to
+168
| #[test] | ||
| fn test_semver_validation_at_init() { | ||
| let env = Env::default(); | ||
| let admin = Address::generate(&env); | ||
| let token = Address::generate(&env); | ||
| let contract_id = env.register_contract(None, SplitEscrowContract); | ||
| let client = SplitEscrowContractClient::new(&env, &contract_id); | ||
|
|
||
| // Valid | ||
| client.initialize(&admin, &token, &String::from_str(&env, "0.1.0")); | ||
| assert_eq!(client.get_version(), String::from_str(&env, "0.1.0")); |
There was a problem hiding this comment.
Missing env.mock_all_auths() before initialize.
The initialize function calls admin.require_auth(). Without mocking authentication, this test should fail on the auth check before reaching the semver validation. Add env.mock_all_auths() to ensure the test validates what it claims.
🐛 Proposed fix
#[test]
fn test_semver_validation_at_init() {
let env = Env::default();
+ env.mock_all_auths();
let admin = Address::generate(&env);
let token = Address::generate(&env);
let contract_id = env.register_contract(None, SplitEscrowContract);
let client = SplitEscrowContractClient::new(&env, &contract_id);
// Valid
client.initialize(&admin, &token, &String::from_str(&env, "0.1.0"));
assert_eq!(client.get_version(), String::from_str(&env, "0.1.0"));
}📝 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.
Suggested change
| #[test] | |
| fn test_semver_validation_at_init() { | |
| let env = Env::default(); | |
| let admin = Address::generate(&env); | |
| let token = Address::generate(&env); | |
| let contract_id = env.register_contract(None, SplitEscrowContract); | |
| let client = SplitEscrowContractClient::new(&env, &contract_id); | |
| // Valid | |
| client.initialize(&admin, &token, &String::from_str(&env, "0.1.0")); | |
| assert_eq!(client.get_version(), String::from_str(&env, "0.1.0")); | |
| #[test] | |
| fn test_semver_validation_at_init() { | |
| let env = Env::default(); | |
| env.mock_all_auths(); | |
| let admin = Address::generate(&env); | |
| let token = Address::generate(&env); | |
| let contract_id = env.register_contract(None, SplitEscrowContract); | |
| let client = SplitEscrowContractClient::new(&env, &contract_id); | |
| // Valid | |
| client.initialize(&admin, &token, &String::from_str(&env, "0.1.0")); | |
| assert_eq!(client.get_version(), String::from_str(&env, "0.1.0")); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/split-escrow/src/test.rs` around lines 158 - 168, The test
test_semver_validation_at_init is missing a mocked auth context so initialize
(which calls admin.require_auth()) will fail before semver checks; fix by
calling env.mock_all_auths() at the start of the test before invoking
SplitEscrowContractClient::initialize (or before any admin.require_auth() path)
so the auth check is satisfied and the semver validation/assertion can run.
Owner
|
You did a clean job but could you resolve the conflicts? |
Owner
Contributor
Author
|
i am so sorry i thought my pull request was successful i will get it done in a giffy |
Owner
|
Hi again @fredericklamar342-prog Can you resolve it so that I can merge your PR and you can get your points? |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
done
Summary by CodeRabbit
New Features
Bug Fixes / Behavior Changes
Tests