Skip to content

feat: restore review timestamps and stabilize tests#51

Merged
Naomi-Gift merged 1 commit intoHubDApp:mainfrom
Amas-01:feat/review-timestamps
Mar 1, 2026
Merged

feat: restore review timestamps and stabilize tests#51
Naomi-Gift merged 1 commit intoHubDApp:mainfrom
Amas-01:feat/review-timestamps

Conversation

@Amas-01
Copy link
Contributor

@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
Contributor 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
Contributor 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!

@Amas-01 Amas-01 force-pushed the feat/review-timestamps branch from 6578a4f to d6f0e4d Compare February 27, 2026 19:45
@Amas-01
Copy link
Contributor Author

Amas-01 commented Mar 1, 2026

I thought this PR can still be merged.
So I'll open a Ticket for my Drips Point to be Implemented

@Naomi-Gift Naomi-Gift merged commit d6f4cad into HubDApp:main Mar 1, 2026
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