Skip to content

feat: restore review timestamps and stabilize tests#51

Open
Amas-01 wants to merge 3 commits intoHubDApp:mainfrom
Amas-01:feat/review-timestamps
Open

feat: restore review timestamps and stabilize tests#51
Amas-01 wants to merge 3 commits intoHubDApp:mainfrom
Amas-01:feat/review-timestamps

Conversation

@Amas-01
Copy link

@Amas-01 Amas-01 commented Feb 25, 2026

Closes #18

Copy link
Contributor

@Naomi-Gift Naomi-Gift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.request_verification ignores errors
In lib.rs, request_verification always returns Ok(()) and does not use the result of VerificationRegistry::request_verification. If the registry ever returns Result<(), Error>, failures will be hidden. Either have the registry return Result<(), Error> and propagate it (e.g. with ?), or document that errors are intentionally not surfaced and add a short // TODO if this is temporary.
2. update_project is a no-op
In project_registry.rs, update_project is stubbed with Ok(()) and does not load the project, check ownership, or write back. Callers will think updates are applied when they are not. Either implement it (load project, require caller == project.owner, apply and persist updates) or return a clear error (e.g. “not implemented”) instead of Ok(()) until it is implemented.
3. Tests must handle Result and match new API
register_one_project
client.register_project(...) returns Result<u64, Error>. Use the returned id (e.g. .unwrap() or .expect(...)) when calling get_project(&id) and in assertions.
test_register_project_success
client.get_project(&id) returns Result<Project, Error>. Use .unwrap() (or equivalent) before using project.name / project.owner.
test_get_fee_config_after_set
client.get_fee_config() returns Result<FeeConfig, Error>. Use .unwrap() (or equivalent) before reading config.verification_fee / config.registration_fee / config.treasury.
test_get_project_none_for_nonexistent_id
With get_project returning Result, a missing project is Err, not Option. Assert on the result (e.g. assert!(client.try_get_project(&999).is_err())) instead of expecting an optional value.
test_multiple_concurrent_registrations_same_user
Ensure each id from register_project is taken from the Result (e.g. .unwrap()), and that building/asserting the list of ids uses the correct Soroban Vec API for SDK 22 (e.g. Vec::from_array or equivalent if that’s the intended API).
4. Restore assertion in test_validation_invalid_project_name_empty
The test currently does let _ = client.try_register_project(...) and does not assert. Add an assertion that the call fails with the expected error (e.g. assert_eq!(..., Err(Ok(Error::InvalidProjectName))) or the appropriate variant your contract uses for empty name).
5. test_verification_flow_approve no longer tests approval
The test was reduced to only setting fee config and calling try_request_verification; it no longer checks the full flow (pay_fee → request → approve → get_verification and status). Either restore the full flow and assert get_verification(...).unwrap().status == VerificationStatus::Verified, or rename the test (e.g. to reflect that it only checks request after fee config) and add a separate test for the full approval flow when the registry supports it.
6. Remove build artifacts from the PR
dongle-smartcontract/target/.rustc_info.json (and any other generated files under target/) should not be committed. Remove them from the PR and ensure target/ is in .gitignore.

@Amas-01
Copy link
Author

Amas-01 commented Feb 25, 2026

Endeavour to merge now, as there's no conflict.

@Naomi-Gift
Copy link
Contributor

@Amas-01 pull , fix ad push again

@Amas-01
Copy link
Author

Amas-01 commented Feb 26, 2026

Why is there always a conflict when it gets to my turn? Fixed all these conflicts earlier this night
I'll fix it tomorrow, not on my system Rn

@Naomi-Gift
Copy link
Contributor

@Amas-01 I am waiting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Include Timestamp for Reviews

2 participants