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

IOS-8312 solana rent notification #4302

Merged
merged 13 commits into from
Nov 29, 2024

Conversation

fedorov-d
Copy link
Contributor

@fedorov-d fedorov-d commented Nov 28, 2024

Добавил нотификацию о недопустимости вывода суммы после которой остаток будет меньше чем minimalBalanceForRentExemption для соланы. Выводить в 0 также допустимо
IMG_A4115504CAAA-1

@fedorov-d fedorov-d requested review from tureck1y and a team as code owners November 28, 2024 08:42
}
.eraseToAnyPublisher()
let amountValue = Amount(with: wallet.blockchain, value: mainAccountRentExemption)
return CurrentValueSubject(amountValue).eraseToAnyPublisher()
Copy link
Contributor

Choose a reason for hiding this comment

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

Интересная запись, а почему не Just()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

поправил

@@ -300,6 +292,29 @@ extension SolanaWalletManager: RentProvider {
}
}

extension SolanaWalletManager: TransactionValidator {
Copy link
Contributor

Choose a reason for hiding this comment

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

лучше бы конечно сделать как и в остальных, просто для консистенстности, так как ни один WalletManager сейчас не реализует TransactionValidator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

поправил

@fedorov-d fedorov-d requested a review from ba1ashov November 29, 2024 11:11
ba1ashov
ba1ashov previously approved these changes Nov 29, 2024
Comment on lines +216 to +228
// MARK: - RentExtemptionRestrictable e.g. SolanaWalletManager

extension TransactionValidator where Self: RentExtemptionRestrictable {
func validate(amount: Amount, fee: Fee, destination: DestinationType) async throws {
Log.debug("TransactionValidator \(self) doesn't checking destination. If you want it, make our own implementation")
try validate(amount: amount, fee: fee)
}

func validate(amount: Amount, fee: Fee) throws {
try validateAmounts(amount: amount, fee: fee.amount)
try validateRentExtemption(amount: amount, fee: fee.amount)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@skibinalexander
Copy link
Contributor

На токене я сам проверю

@fedorov-d fedorov-d requested a review from Andoran90 November 29, 2024 11:26
*/
.flatMap { service, info in
service.minimalBalanceForRentExemption(dataLength: info.space ?? 0)
.tryMap { rentExemption in
Copy link
Contributor

Choose a reason for hiding this comment

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

а зачем tryMap, а не просто map?


let remainingBalance = balance.value - (amount.value + fee.value)

if remainingBalance > 0, remainingBalance < minimalAmountForRentExemption.value {
Copy link
Contributor

Choose a reason for hiding this comment

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

а тут точно нужна проверка что больше 0?
Кстати, если сейчас выводить все, тогда остаточный баланс будет 0 и проверка не сработает. Не звучит как норм.
Вообще выглядит будто надо оставить только 2 проверку или же сделать в начале проверку на то что остаток 0 или больше 0, а потом уже проверку на минималку.

А то это же можно вызвать и не через TransactionValidator и тогда отрицательное значение и 0 будет валидным остатоком

Copy link
Contributor Author

Choose a reason for hiding this comment

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

в ноль выводить можно, не указал в описании

Copy link
Contributor Author

Choose a reason for hiding this comment

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

добавил доп проверку

Copy link
Contributor

Choose a reason for hiding this comment

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

Вообще вроде как это должно в validateAmounts проверяться, но хз, лишним не будет наверно

Andoran90
Andoran90 previously approved these changes Nov 29, 2024
@fedorov-d fedorov-d merged commit a3f0a44 into develop Nov 29, 2024
5 checks passed
@fedorov-d fedorov-d deleted the features/staking/IOS-8312_solana_rent_notification branch November 29, 2024 13:00
Andoran90 pushed a commit that referenced this pull request Dec 6, 2024
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.

4 participants