fix_: make member use wallet tokens during permission checking #5268
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (71)
|
kounkou
force-pushed
the
fix-make-member-use-wallet-for-permission-checking
branch
6 times, most recently
from
fa1c7eb to
7b73915
Compare
jrainville
reviewed
May 31, 2024
There was a problem hiding this comment.
Great job! Just small comments.
I'll also wait for the others to review since they know this code better.
FYI, @osmaczko did a PR also on that code. Hopefully it's doesn't conflict too much.
I don't think you need to also implement the getOwnersForCollection function from cache, since it's only for control nodes, but it's worth noting.
kounkou
force-pushed
the
fix-make-member-use-wallet-for-permission-checking
branch
5 times, most recently
from
a402d70 to
4853508
Compare
mprakhov
reviewed
Jun 3, 2024
mprakhov
reviewed
Jun 3, 2024
| @@ -396,6 +398,61 @@ func (o *OwnershipDB) Update(chainID w_common.ChainID, ownerAddress common.Addre | |||
| return | |||
| } | |||
|
|
|||
| func (o *OwnershipDB) FetchCachedCollectibleOwnersByContractAddress(chainID w_common.ChainID, contractAddress common.Address) (*thirdparty.CollectibleContractOwnership, error) { | |||
There was a problem hiding this comment.
could you please add a test for this fn... what should we get, when info is not found?
There was a problem hiding this comment.
sure, please note that this is exactly the same behavior as for FetchBalancesByOwnerAndContractAddress. Instead of looking on chain we just get the data in cache, but let me have a look if I can add a test that makes sense if not already implemented somewhere.
mprakhov
reviewed
Jun 3, 2024
kounkou
force-pushed
the
fix-make-member-use-wallet-for-permission-checking
branch
from
4853508 to
316b50c
Compare
|
@kounkou hey, is it the change you wanted to be tested? If so, please create corresponding PR in desktop repo |
Hey @anastasiyaig please let me follow up with you once I have completed the merging of changes from @osmaczko PR. Should be achieve early next week. |
kounkou
force-pushed
the
fix-make-member-use-wallet-for-permission-checking
branch
from
316b50c to
7ba2a68
Compare
kounkou
force-pushed
the
fix-make-member-use-wallet-for-permission-checking
branch
from
7ba2a68 to
a5f0f70
Compare
kounkou
force-pushed
the
fix-make-member-use-wallet-for-permission-checking
branch
2 times, most recently
from
8151b35 to
3363e81
Compare
…checking fixes #14913
kounkou
force-pushed
the
fix-make-member-use-wallet-for-permission-checking
branch
from
3363e81 to
b896f0a
Compare
|
During the testing, @glitchminer saw a problem, but it's actually not caused by this pr, it is already known issue when trying to use the same collectible multiple times when creating a permission : |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
During the checking of permissions that happens either on edit shared addresses or during the blurred screen when joining a community, the member will perform some permissions checking. Currently the permission checking is done by fetching tokens from the chain which is wasteful in terms of resources. These changes aims at using the cached tokens from the wallet which improves the speed of tokens retrieval and improve the user experience.
fixes #14913
Acceptance criteria
In progressImportant changes:
Closes status-im/status-desktop#14913
Manual testing
Please notince how fast the evaluation is compared to before
Before
Screencast.from.2024-06-02.01-17-01.webm
After
Screencast.from.2024-06-02.01-59-40.webm