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

feat(x/asset): Add clawback and transfer auth priviledges #176

Open
wants to merge 9 commits into
base: example-priv
Choose a base branch
from

Conversation

sontrinh16
Copy link

No description provided.

@jiujiteiro jiujiteiro requested a review from facs95 September 16, 2024 12:28
@jiujiteiro
Copy link
Collaborator

@facs95 can you take a look at this please

return newToAddr, nil
}

checker := k.RestrictionChecker[0]
Copy link
Contributor

@haleyga haleyga Sep 17, 2024

Choose a reason for hiding this comment

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

There is a comment in x/asset/keeper/keeper.go that we should not support more than 1 RestrictionChecker. However, the code seems to allow more than one anyway (both x/asset/keeper/keeper.go and x/asset/keeper/restriction.go have the conditional check len(k.RestrictionChecker) == 0 instead of something like len(k.RestrictionChecker) != 1 for instance.

This line checker := k.RestrictionChecker[0] will ignore any RestrictionChecker after the first one registered. If multiple exist, how can we be sure the first is the correct one to use? It seems the code should either enforce only one RestrictionChecker, or this code block should iterate all RestrictionCheckers in this list.

x/asset/priviledges/clawback/types.go Show resolved Hide resolved
x/asset/priviledges/transfer_auth/priv.go Outdated Show resolved Hide resolved
x/asset/priviledges/transfer_auth/priv.go Outdated Show resolved Hide resolved
Comment on lines +118 to +137
for k := range m.Addrs {
v := m.Addrs[k]
baseI := i
i--
if v {
dAtA[i] = 1
} else {
dAtA[i] = 0
}
i--
dAtA[i] = 0x10
i -= len(k)
copy(dAtA[i:], k)
i = encodeVarintPriv(dAtA, i, uint64(len(k)))
i--
dAtA[i] = 0xa
i = encodeVarintPriv(dAtA, i, uint64(baseI-i))
i--
dAtA[i] = 0xa
}

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism
Comment on lines +160 to +165
for k, v := range m.Addrs {
_ = k
_ = v
mapEntrySize := 1 + len(k) + sovPriv(uint64(len(k))) + 1 + 1
n += mapEntrySize + 1 + sovPriv(uint64(mapEntrySize))
}

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism
Comment on lines +118 to +137
for k := range m.Addrs {
v := m.Addrs[k]
baseI := i
i--
if v {
dAtA[i] = 1
} else {
dAtA[i] = 0
}
i--
dAtA[i] = 0x10
i -= len(k)
copy(dAtA[i:], k)
i = encodeVarintRestriction(dAtA, i, uint64(len(k)))
i--
dAtA[i] = 0xa
i = encodeVarintRestriction(dAtA, i, uint64(baseI-i))
i--
dAtA[i] = 0xa
}

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism
Comment on lines +160 to +165
for k, v := range m.Addrs {
_ = k
_ = v
mapEntrySize := 1 + len(k) + sovRestriction(uint64(len(k))) + 1 + 1
n += mapEntrySize + 1 + sovRestriction(uint64(mapEntrySize))
}

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism
Comment on lines +36 to +49
for priv, restrictionChecker := range k.RestrictionChecker {
if slices.Contains(enabledPrivileges, priv) {
isAllow, err := restrictionChecker.IsAllow(ctx, tokenID, fromAddr.String())
if err != nil {
return newToAddr, err
}
if isAllow {
continue
} else { //nolint:revive // superfluous else, could fix, but not worth it?
err = errorsmod.Wrapf(types.ErrNotAuthorized, "%s is not authorized to transact with %s", fromAddr, coin.Denom)
return newToAddr, err
}
}
}

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism
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.

5 participants