Skip to content

refactor(ruby): idiomatic pass (Shopify Ruby + rubystyle)#223

Merged
lgalabru merged 1 commit into
fix/ruby-security-hardeningfrom
refactor/ruby-idiomatic
Jul 9, 2026
Merged

refactor(ruby): idiomatic pass (Shopify Ruby + rubystyle)#223
lgalabru merged 1 commit into
fix/ruby-security-hardeningfrom
refactor/ruby-idiomatic

Conversation

@EfeDurmaz16

@EfeDurmaz16 EfeDurmaz16 commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Idiomatic-Ruby pass over the Ruby SDK MPP verifier (Shopify Ruby Style Guide + rubystyle.guide + the pay-kit Ruby review overlay). Behavior-preserving — reject strings and verifier checks are unchanged; verified by the full suite.

Stacked on #222 (fix(ruby): ...): base is fix/ruby-security-hardening so this PR shows only the idiomatic delta; it re-targets #219 once #222 merges. This is the refactor(<lang>) half of the per-language work.

  • mpp/protocol/solana/verifier.rb: explicit program tables (TOKEN_PROGRAMS / ALLOWED_PAYMENT_PROGRAMS), three keyword_init Struct value objects (TransferExpectation / SplTransferExpectation / AtaCreationPolicy), and keyword / value-object flow over long positional helper calls.

Scoped strictly to structural refactoring: the earlier solana.rb rescue narrowing was reverted after review (a behavior change belongs in a fix(ruby), not a refactor), so the diff is a single file.

Verified: bundle exec rake test → 465 runs, 0 failures; standardrb clean.

@greptile-apps

greptile-apps Bot commented Jul 9, 2026

Copy link
Copy Markdown

Greptile Summary

This PR is a behavior-preserving idiomatic Ruby refactor of verifier.rb, stacked on the security-hardening base branch. No logic changes are introduced — all existing verification paths, error messages, and rejection conditions are unchanged.

  • Extracts two frozen constant arrays (TOKEN_PROGRAMS, ALLOWED_PAYMENT_PROGRAMS) to replace inline anonymous arrays that were re-allocated on every call.
  • Introduces three keyword_init: true Struct value objects (TransferExpectation, SplTransferExpectation, AtaCreationPolicy) to replace long positional argument lists.
  • Adds sol_transfer_expectations and spl_transfer_expectations factory helpers that collapse two call sites into a single uniform each loop.

Confidence Score: 5/5

Safe to merge — every verification path, rejection condition, and error message is unchanged from the base branch.

The diff is a single file, purely structural: frozen constant arrays replace inline anonymous arrays, three keyword_init Structs replace positional argument lists, and two factory helpers unify call sites. No conditional logic, guard clause, or error-path was added, removed, or reordered in a security-relevant way. The 465-test suite passes cleanly.

No files require special attention.

Important Files Changed

Filename Overview
ruby/lib/pay_kit/protocols/mpp/protocol/solana/verifier.rb Idiomatic Ruby refactor: frozen constant arrays, three keyword_init Struct value objects, and factory helpers replace positional arg lists; logic is unchanged and test suite confirms parity.

Reviews (4): Last reviewed commit: "refactor(ruby): idiomatic pass (Shopify ..." | Re-trigger Greptile

begin
rpc.account_owner(mint)
rescue => error
rescue ::PayCore::Solana::Rpc::RpcError, RuntimeError, KeyError, JSON::ParserError, TypeError => error

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Explicit rescue list may miss network-level exceptions

The new rescue clause covers RpcError, RuntimeError, KeyError, JSON::ParserError, and TypeError, but Ruby's Errno::* family (e.g., Errno::ECONNREFUSED, Errno::ETIMEDOUT), Net::ReadTimeout, SocketError, and OpenSSL::SSL::SSLError are all StandardError subclasses that are not subclasses of RuntimeError. If ::PayCore::Solana::Rpc::RpcError does not wrap every low-level transport failure, those exceptions will now escape this block and surface as unhandled 500s instead of the clean Mpp::Error translation that the old bare rescue guaranteed. Please verify that RpcError wraps all failure modes of rpc.account_owner, or widen the list with the additional transport-layer exception classes.

Behavior-preserving idiomatic cleanup of the Ruby SDK MPP verifier:
- mpp/protocol/solana/verifier.rb: explicit program tables
  (TOKEN_PROGRAMS / ALLOWED_PAYMENT_PROGRAMS), three keyword-init Struct value
  objects (TransferExpectation / SplTransferExpectation / AtaCreationPolicy),
  and keyword / value-object flow over long positional helper calls. Reject
  strings and verifier checks are unchanged.

Verified: 465 runs 0 failures; standardrb clean.
@EfeDurmaz16

Copy link
Copy Markdown
Collaborator Author

Addressed the 4/5 review: reverted the solana.rb rescue narrowing (a behavior change doesn't belong in a refactor, and RpcError's rpc_error_class hook means transport-error wrapping isn't guaranteed for every rpc instance). The PR is now a single-file, purely structural verifier.rb refactor — the part the review already rated clean. 465 tests green, standardrb clean. @greptile-apps please review

@EfeDurmaz16

Copy link
Copy Markdown
Collaborator Author

Re-review is clean on cb392c3 (the earlier 4/5 note was the solana.rb rescue narrowing, now reverted — this PR is a single-file, purely structural verifier.rb refactor). Ready alongside #222. @lgalabru

@EfeDurmaz16

Copy link
Copy Markdown
Collaborator Author

@greptile-apps please review

@lgalabru
lgalabru merged commit 45fcc99 into fix/ruby-security-hardening Jul 9, 2026
29 of 44 checks passed
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.

2 participants