Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add prefix response support for Gemini wallets #3237

Merged
merged 5 commits into from May 24, 2021
Merged

Add prefix response support for Gemini wallets #3237

merged 5 commits into from May 24, 2021

Conversation

tsmartt
Copy link
Collaborator

@tsmartt tsmartt commented May 21, 2021

We need to tell the browser which Gemini wallet a user has for direct deposits. We use the protobuf prefix responses for this, and this PR adds Gemini wallet support for that.

Closes brave-intl/creators-private-issues#278

@tsmartt tsmartt changed the title WIP: Add prefix response support for Gemini wallets Add prefix response support for Gemini wallets May 21, 2021
@tsmartt tsmartt marked this pull request as ready for review May 21, 2021 19:07
yachtcaptain23
yachtcaptain23 reviewed May 21, 2021
View reviewed changes
app/jobs/cache/browser_channels/responses_for_prefix.rb Outdated
@@ -21,7 +21,7 @@ def perform(prefix)
def generate_brotli_encoded_channel_response(prefix:)
@site_banner_lookups = SiteBannerLookup.where("sha2_base16 LIKE ?", prefix + "%")
channel_responses = PublishersPb::ChannelResponseList.new
@site_banner_lookups.includes(publisher: :uphold_connection).includes(publisher: :paypal_connection).each do |site_banner_lookup|
@site_banner_lookups.includes(publisher: [:uphold_connection, :bitflyer_connection, :gemini_connection, :paypal_connection]).each do |site_banner_lookup|
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@site_banner_lookups.includes(publisher: [:uphold_connection, :bitflyer_connection, :gemini_connection, :paypal_connection]).each do |site_banner_lookup|
@site_banner_lookups.includes(publisher: [:uphold_connection, :bitflyer_connection, :gemini_connection]).each do |site_banner_lookup|

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea! I had initially removed all paypal support from this PR, but reverted it since I wanted to keep the concepts separate. I want to put up a new PR that only removes Paypal support.

@tsmartt
Copy link
Collaborator Author

tsmartt commented May 21, 2021

By the way this works using git@github.com:zenparsing/publist-tools.git

Just update the pcdn links and proto files:

└─[$] <git:(master*)> node cli.js fetch --id "jumde.github.io" --env development
{
  url: 'https://pcdn.brave.software/publishers/prefixes/0d62',
  channelInfo: {
    wallets: [
      {
        geminiWallet: { walletState: 'GEMINI_ACCOUNT_NO_KYC', address: '123' },
        provider: 'geminiWallet'
      }
    ],
    channelIdentifier: 'jumde.github.io',
    siteBannerDetails: {
      donationAmounts: [],
      title: '',
      description: '',
      backgroundUrl: '',
      logoUrl: '',
      socialLinks: null
    }
  }
}

tsmartt and others added 2 commits May 21, 2021 15:44
yachtcaptain23
yachtcaptain23 reviewed May 24, 2021
View reviewed changes
test/jobs/cache/browser_channels/responses_for_prefix_test.rb
@@ -20,10 +20,28 @@ def self.test_order
assert service.temp_file.present?
result = Brotli.inflate(File.open(service.temp_file.path, 'rb').readlines.join("").slice(4..-1))
result = PublishersPb::ChannelResponseList.decode(result)
assert result.channel_responses[0].wallets[0].uphold_wallet.address
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert result.channel_responses[0].wallets[0].uphold_wallet.address

The next line already does a null check, so this isn't needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not a null check, it's an equality check. They could both be null.

@yachtcaptain23
Copy link
Contributor

Leaving as a note, the corresponding protobuf file change in brave-core is in https://github.com/brave/brave-core/blob/e618e80cc01c0de34f846e5eab01c0e094f6547e/vendor/bat-native-ledger/src/bat/ledger/internal/publisher/protos/channel_response.proto

@tsmartt
Copy link
Collaborator Author

tsmartt commented May 24, 2021 •
edited

the corresponding protobuf file change in brave-core is in

Thanks. PJ is working on incorporating the changes.

@tsmartt tsmartt merged commit 19e0bf7 into brave-intl:staging May 24, 2021
@tsmartt tsmartt deleted the add-gemini-channels branch May 24, 2021 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yachtcaptain23 yachtcaptain23 yachtcaptain23 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tsmartt @yachtcaptain23