Skip to content

fix: propose join and rename to submit_participant_info#851

Merged
kevindeforth merged 5 commits intomainfrom
kd/842-fix-propose-join
Aug 15, 2025
Merged

fix: propose join and rename to submit_participant_info#851
kevindeforth merged 5 commits intomainfrom
kd/842-fix-propose-join

Conversation

@kevindeforth
Copy link
Copy Markdown
Contributor

Resolves #841 and contract part of #842

@kevindeforth kevindeforth marked this pull request as ready for review August 15, 2025 09:38
Copy link
Copy Markdown
Collaborator

@netrome netrome left a comment

Choose a reason for hiding this comment

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

Good stuff! Love these small reviews and clear issues. Makes it so much easier to squeeze in a small PR review before lunch 🤩

Would love to see tests for this, but I see you created a follow-up for that already since it requires the attestation module to be integrated. #846

Otherwise, just one question about a clone.

Comment thread libs/chain-signatures/contract/src/tee/tee_state.rs Outdated
@kevindeforth kevindeforth added this pull request to the merge queue Aug 15, 2025
@ghost ghost removed this pull request from the merge queue due to a manual request Aug 15, 2025
@ghost ghost self-requested a review August 15, 2025 09:58
Comment thread libs/chain-signatures/contract/src/tee/tee_state.rs Outdated
Comment thread libs/chain-signatures/contract/src/tee/tee_state.rs Outdated
@netrome
Copy link
Copy Markdown
Collaborator

netrome commented Aug 15, 2025

@near-bookrock - why did you remove this PR from the merge queue?
image

Copy link
Copy Markdown
Collaborator

@netrome netrome left a comment

Choose a reason for hiding this comment

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

Looks better now, thank you! 🙏

@ghost
Copy link
Copy Markdown

ghost commented Aug 15, 2025

@near-bookrock - why did you remove this PR from the merge queue? image

I wanted to have time to also review before it got merged. https://nearone.slack.com/archives/C0912BTG51T/p1755252073726839?thread_ts=1755250728.252599&cid=C0912BTG51T.

It was opened at 11:38 GMT+2 and moved to merge queue at 11:54 GMT+2, so there wasn't much time to review if I also wanted to take a look.

@netrome
Copy link
Copy Markdown
Collaborator

netrome commented Aug 15, 2025

I wanted to have time to also review before it got merged. https://nearone.slack.com/archives/C0912BTG51T/p1755252073726839?thread_ts=1755250728.252599&cid=C0912BTG51T.

I see, fair enough.

It was opened at 11:38 GMT+2 and moved to merge queue at 11:54 GMT+2, so there wasn't much time to review if I also wanted to take a look.

Yep, this is the gold standard imo. Small PRs and quick review turnaround. Nothing wrong with post-merge reviews and follow-ups either. But in this case when you're reviewing just as it got added to the merge queue I agree it can be reasonable to hold the merge for a short bit.

@kevindeforth kevindeforth added this pull request to the merge queue Aug 15, 2025
@kevindeforth
Copy link
Copy Markdown
Contributor Author

I wanted to have time to also review before it got merged. https://nearone.slack.com/archives/C0912BTG51T/p1755252073726839?thread_ts=1755250728.252599&cid=C0912BTG51T.

I see, fair enough.

It was opened at 11:38 GMT+2 and moved to merge queue at 11:54 GMT+2, so there wasn't much time to review if I also wanted to take a look.

Yep, this is the gold standard imo. Small PRs and quick review turnaround. Nothing wrong with post-merge reviews and follow-ups either. But in this case when you're reviewing just as it got added to the merge queue I agree it can be reasonable to hold the merge for a short bit.

Yeah definitely, Much appreciated @near-bookrock removed it, as it resulted in better and more maintainable code!

Merged via the queue into main with commit 11893e3 Aug 15, 2025
14 checks passed
@kevindeforth kevindeforth deleted the kd/842-fix-propose-join branch August 15, 2025 17:34
Comment thread libs/chain-signatures/contract/src/lib.rs
Comment thread libs/chain-signatures/contract/src/tee/tee_state.rs
Comment thread libs/chain-signatures/contract/src/tee/tee_state.rs
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.

Fix propose_join

3 participants