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

December 2024 Fixes #28

Closed
0x4007 opened this issue Dec 4, 2024 · 18 comments · Fixed by #29
Closed

December 2024 Fixes #28

0x4007 opened this issue Dec 4, 2024 · 18 comments · Fixed by #29

Comments

@0x4007
Copy link
Member

0x4007 commented Dec 4, 2024

Registration failures makes the system unusable.

Unnecessary Error

The previous error above it is sufficient this one should not display to the user.

! Error: Error: No wallet address found

Originally posted by @ubiquity-os[bot] in ubiquity/business-development#85 (comment)

Registration Failure

! Error: [object Object]

Originally posted by @ubiquity-os[bot] in ubiquity/business-development#85 (comment)

Copy link
Contributor

@gentlementlegen the deadline is at Thu, Dec 5, 5:01 AM UTC

@gentlementlegen
Copy link
Member

Another instance of the error: ubiquity-os/plugins-wishlist#31 (comment)

Copy link
Contributor

Note

The following contributors may be suitable for this task:

rndquu

1% Match ubiquity/audit.ubq.fi#16

@gentlementlegen
Copy link
Member

The ! Error: Error: No wallet address found comes from command start stop not this plugin I think.

@gentlementlegen
Copy link
Member

Link to the error in CF: https://dash.cloudflare.com/17b9dfa79e16b79dffcb11a66768539c/workers/services/view/ubiquity-os-command-wallet-main/production/observability/logs?granularity=0&time=%7B%22type%22%3A%22absolute%22%2C%22to%22%3A1733308320000%2C%22from%22%3A1733308200000%7D

The error is the following:

�[33m	⚠ {
	    "err": {
	      "code": "23505",
	      "details": "Key (address)=(0xB13260bfEe08DcA208F2ECc735171B21763EaaF6) already exists.",
	      "hint": null,
	      "message": "duplicate key value violates unique constraint \"new_wallets_wallet_key\""
	    },
	    "caller": "plugin"
	  }�[0m

I believe the main problem is the error display that is wrong (should give this instead of [Object object]).

@gentlementlegen
Copy link
Member

@0x4007 The root of the error is that the wallet was already in our DB. Do we allow an address to be linked to multiple accounts? Shall we block this behavior?

@0x4007
Copy link
Member Author

0x4007 commented Dec 6, 2024

I see that's interesting. It's hard to say what's the right answer but I think we should not support sybils. To use the system you should only be using one GitHub account. There will be an incentive to use a single account when we have XP one day.

You can make the error message clear, saying that the wallet is already registered. I suppose we can also make a note that they can unset their wallet address on the old account before setting on their new account in case they want to change the account that they use?

@gentlementlegen
Copy link
Member

gentlementlegen commented Dec 6, 2024

Well that's the thing I don't think there is any way to "unset" wallets at this time. We could also implement that. I don't think it's a very common use case, only if you use multiple accounts or somehow lost your account and you use a new one? Here is a complete error for a duplicated account: Meniole#5 (comment)

So I'd say either we allow multiple accounts linked to one wallet, either we offer a way to unset wallets from an account.

@0x4007
Copy link
Member Author

0x4007 commented Dec 6, 2024

It's useful for us to know who is a sybil. Maybe unsetting can be like

/wallet null

@gentlementlegen
Copy link
Member

gentlementlegen commented Dec 7, 2024

Okay then I would suggest something like /wallet --unset to avoid ambiguity with domain names.

Copy link
Contributor

+ Evaluating results. Please wait...

Copy link
Contributor

 [ 100 WXDAI ] 

@gentlementlegen
Contributions Overview
ViewContributionCountReward
IssueTask1100
Conversation Incentives
CommentFormattingRelevancePriorityReward

 [ 100 WXDAI ] 

@0x4007
⚠️ Your rewards have been limited to the task price of 100 WXDAI.
Contributions Overview
ViewContributionCountReward
IssueSpecification137
IssueComment223.944
ReviewComment645.944
Conversation Incentives
CommentFormattingRelevancePriorityReward
Registration failures makes the system unusable. # Unnecessary…
9.25
content:
  content:
    p:
      score: 0
      elementCount: 4
    h1:
      score: 1
      elementCount: 2
    em:
      score: 0
      elementCount: 2
    a:
      score: 5
      elementCount: 1
  result: 7
regex:
  wordCount: 39
  wordValue: 0.1
  result: 2.25
1437
I see that's interesting. It's hard to say what's the right answ…
5.05
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 101
  wordValue: 0.1
  result: 5.05
1420.2
It's useful for us to know who is a sybil. Maybe unsetting can b…
1.17
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 18
  wordValue: 0.1
  result: 1.17
0.843.744
Code looks fine just need to include my QA comment adjustments. …
1.06
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 16
  wordValue: 0.1
  result: 1.06
0.542.12
You can probably set it to null.
0.52
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 7
  wordValue: 0.1
  result: 0.52
0.641.248
I realized that this double hyphen is really only geared towards…
1.33
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 21
  wordValue: 0.1
  result: 1.33
0.743.724
Yes locations metadata caused things to be unnecessarily complic…
2.15
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 37
  wordValue: 0.1
  result: 2.15
0.443.44
Did you test that this works? My original belief was that we wou…
4.67
content:
  content:
    p:
      score: 0
      elementCount: 2
  result: 0
regex:
  wordCount: 92
  wordValue: 0.1
  result: 4.67
0.8414.944
I left some feedback there https://github.com/Meniole/command-wa…
5.39
content:
  content:
    p:
      score: 0
      elementCount: 1
    a:
      score: 5
      elementCount: 1
  result: 5
regex:
  wordCount: 5
  wordValue: 0.1
  result: 0.39
0.3420.468

@gentlementlegen
Copy link
Member

For some reasons it seems that @whilefoo comments didn't get picked up here? I will have a look.

@0x4007
Copy link
Member Author

0x4007 commented Dec 12, 2024

They have had problems registering their wallet in the past so that may also be related.

Lastly we still need to adjust the algorithm to not take a baseline from the formatting score. If relevance is 0 then the final score should be 0

@gentlementlegen
Copy link
Member

@0x4007 That change was made after your request: ubiquity-os-marketplace/text-conversation-rewards#92 (comment)

I will change it back.

@0x4007
Copy link
Member Author

0x4007 commented Dec 13, 2024

@0x4007 That change was made after your request: ubiquity-os-marketplace/text-conversation-rewards#92 (comment)

I will change it back.

We have come a long way in a few months.

There were a lot of problems when that was posted and I suppose I figured that would have been the quickest solution to slightly more accurate scoring. These days I think the scoring is much more accurate.

Semi related to that but relevance currently doesn't understand image contents. Check this spec, specifically for vector embeddings but maybe we should write in the HTML comments the descriptions of the images?

It could also be really interesting to pull the text contents from links to also score their relevance.

@gentlementlegen
Copy link
Member

@0x4007 you mean that when a user adds an image within a comment, it should also add some description within it? Like so?

@0x4007 profile pic

@rndquu rndquu removed this from Development Dec 13, 2024
@0x4007
Copy link
Member Author

0x4007 commented Dec 13, 2024

@0x4007 you mean that when a user adds an image within a comment, it should also add some description within it? Like so?

@0x4007 profile pic

This should be discussed in the dedicated task

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

Successfully merging a pull request may close this issue.

2 participants